Only count a key as an ancestor if there is a separator #141

Stebalien · 2019-12-03T17:00:15Z

Also make sure to clean and normalize keys before using them as prefixes.

BREAKING CHANGES:

myds.Query(Query{Prefix:"/foo"}) will no longer match "/foobar" (or even "/foo"). This is usually what the user expects, we had a tendency to normalize "/foo/" to "/foo" (when we clean keys), and many datastores can't efficiently search for prefixes that aren't on path-boundaries anyways.
Given a mount datastore with mounts ["/foo", "/"], myds.Put("/foo", "bar") will put the value to the datastore mounted at "/", not "/foo", as the key "/" and "" usually doesn't make sense.

While technically breaking, these changes are much more likely to fix bugs than they are to break things.

key.go

Stebalien · 2020-02-05T23:26:36Z

mount/mount.go

-		}
-
-		if strings.HasPrefix(p, key.String()) {
+		if m.Prefix.IsDescendantOf(key) {


I'm now using proper key functions.

Are the comments above this function still accurate? What should happen with /ao/e/ here?

I'm not really sure what that comment is trying to say (what's A, B?). It would return the /ao/e/ mount point, and / as "rest". I've fixed the documentation and added a test.

Stebalien · 2020-02-05T23:33:35Z

mount/mount.go

+			// We've found an ancestor (or equal) key. We might have
+			// more general datastores, but they won't contain keys
+			// with this prefix so there's no point in searching them.
+			break


This is #140 (comment). Thinking back on this, I think it was a bug.

Given the mounts ["/foo", "/"], lookupAll("/foo/bar") would return both (there's now a test for this). However, it should have only returned "/foo" because no keys under "/foo/bar" would end up in "/".

Note: this may have been impacting the performance of GC.

seems reasonable, this is a contract change but if it increases performance/reliability and no one is utilizing the obscure case then we may as well go for it.

Are we pretty confident that we're not doing queries for things like /blocks even though /blocks is mounted over /?

We are, but that should be fine. That is, we're querying for blocks in /blocks, not for blocks in /. As of this change, lookupAll("/blocks"), should return only the datastore mounted at /blocks.

Stebalien · 2020-02-05T23:34:19Z

mount/mount.go

-
-		if mountPts[i].Equal(prefix) || suffix.String() != "/" {
-			return nil
-		}


The fix in lookupAll should make this unnecessary.

Stebalien · 2020-02-05T23:35:02Z

query/query_impl.go

+		if prefix != "/" {
+			prefix += "/"
+		}
+		qr = NaiveFilter(qr, FilterKeyPrefix{prefix})


We weren't cleaning/fixing keys.

…arator Also make sure to clean and normalize keys before using them as prefixes. BREAKING CHANGES: * `myds.Query(Query{Prefix:"/foo"})` will no longer match "/foobar" (or even "/foo"). This is usually what the user expects, we had a tendency to normalize "/foo/" to "/foo" (when we clean keys), and many datastores can't efficiently search for prefixes that aren't on path-boundaries anyways. * Given a mount datastore with mounts `["/foo", "/"]`, `myds.Put("/foo", "bar")` will put the value to the datastore mounted at "/", not "/foo", as the key "/" and "" usually doesn't make sense. While technically breaking, these changes are much more likely to fix bugs than they are to break things.

Stebalien · 2020-02-05T23:51:06Z

myds.Query(Query{Prefix:"/foo"}) will no longer match "/foobar" (or even "/foo"). This is usually what the user expects, we had a tendency to normalize "/foo/" to "/foo" (when we clean keys), and many datastores can't efficiently search for prefixes that aren't on path-boundaries anyways.

Note: if we don't do this, we need to remember to fix the key transform datastore.

Stebalien · 2020-02-06T00:30:55Z

@whyrusleeping says

uhhhh, i think normalization seems fine to me

whyrusleeping · 2020-02-06T00:34:42Z

Yeah, seems fine to me. We can add more docs around our key model to compensate for the potential weirdness

aschmahmann

Overall LGTM. Left a few comments and requests for clarification

aschmahmann · 2020-02-10T10:54:32Z

query/query_test.go

@@ -88,14 +88,12 @@ func TestNaiveQueryApply(t *testing.T) {

 	q = Query{
 		Limit:  3,


Limit isn't being tested anymore since we only have one result

I've added some more keys to improve the tests.

aschmahmann · 2020-02-10T11:52:46Z

mount/mount.go

-		}
-
-		if strings.HasPrefix(p, key.String()) {
+		if m.Prefix.IsDescendantOf(key) {


Are the comments above this function still accurate? What should happen with /ao/e/ here?

aschmahmann · 2020-02-10T11:57:34Z

mount/mount.go

+			// We've found an ancestor (or equal) key. We might have
+			// more general datastores, but they won't contain keys
+			// with this prefix so there's no point in searching them.
+			break


seems reasonable, this is a contract change but if it increases performance/reliability and no one is utilizing the obscure case then we may as well go for it.

Are we pretty confident that we're not doing queries for things like /blocks even though /blocks is mounted over /?

aschmahmann · 2020-02-10T12:04:02Z

mount/mount.go

@@ -39,7 +39,7 @@ var _ ds.Datastore = (*Datastore)(nil)

 func (d *Datastore) lookup(key ds.Key) (ds.Datastore, ds.Key, ds.Key) {
 	for _, m := range d.mounts {
-		if m.Prefix.Equal(key) || m.Prefix.IsAncestorOf(key) {
+		if m.Prefix.IsAncestorOf(key) {


To clarify, is the intent here that if I've mounted ["/", "/foo"] and then I do a put to /foo that the key will end up in the / mount instead of the /foo mount?

Is there any reason to support this or would we be safer just throwing an error instead?

This should have already been committed...

Stebalien assigned aschmahmann Dec 3, 2019

aschmahmann reviewed Dec 9, 2019

View reviewed changes

key.go Outdated Show resolved Hide resolved

Stebalien force-pushed the fix/ancestor-sep branch from d47cece to 954d9e2 Compare February 5, 2020 19:59

Stebalien unassigned aschmahmann Feb 5, 2020

Stebalien marked this pull request as ready for review February 5, 2020 20:00

Stebalien force-pushed the fix/ancestor-sep branch from 954d9e2 to 7a1443d Compare February 5, 2020 23:25

Stebalien mentioned this pull request Feb 5, 2020

Support Asynchronous Writing Datastores #140

Merged

6 tasks

Stebalien commented Feb 5, 2020

View reviewed changes

Stebalien force-pushed the fix/ancestor-sep branch from 7a1443d to 5598edf Compare February 5, 2020 23:49

Stebalien requested review from alanshaw and whyrusleeping February 6, 2020 00:08

doc: update and improve query docs

d009ffb

aschmahmann approved these changes Feb 10, 2020

View reviewed changes

Stebalien added 3 commits February 10, 2020 15:28

doc(mount): fix and improve documentation

bffc5fd

test: add a lookup test

ae428d8

This should have already been committed...

test(query): be thurough

2fe8b68

Stebalien merged commit 83f9a95 into master Feb 11, 2020

Stebalien deleted the fix/ancestor-sep branch February 11, 2020 00:14

alanshaw requested review from achingbrain and removed request for alanshaw February 24, 2020 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only count a key as an ancestor if there is a separator #141

Only count a key as an ancestor if there is a separator #141

Stebalien commented Dec 3, 2019 •

edited

Loading

Stebalien Feb 5, 2020

aschmahmann Feb 10, 2020

Stebalien Feb 10, 2020

Stebalien Feb 5, 2020

Stebalien Feb 5, 2020

aschmahmann Feb 10, 2020

Stebalien Feb 10, 2020

Stebalien Feb 5, 2020

Stebalien Feb 5, 2020

Stebalien commented Feb 5, 2020

Stebalien commented Feb 6, 2020

whyrusleeping commented Feb 6, 2020

aschmahmann left a comment

aschmahmann Feb 10, 2020

Stebalien Feb 10, 2020

aschmahmann Feb 10, 2020

aschmahmann Feb 10, 2020

aschmahmann Feb 10, 2020

		@@ -88,14 +88,12 @@ func TestNaiveQueryApply(t *testing.T) {

		q = Query{
		Limit: 3,

Only count a key as an ancestor if there is a separator #141

Only count a key as an ancestor if there is a separator #141

Conversation

Stebalien commented Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Feb 5, 2020

Stebalien commented Feb 6, 2020

whyrusleeping commented Feb 6, 2020

aschmahmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Dec 3, 2019 •

edited

Loading