Adds longest prefix matching for custom indexes. #20
Conversation
slackpad
changed the title
| t.Fatalf("err: %v", err) | ||
| } | ||
| if raw != obj1 { | ||
| t.Fatalf("should be nil") |
There was a problem hiding this comment.
aw - cut n' paste - sorry!
| @@ -334,6 +334,39 @@ func (txn *Txn) First(table, index string, args ...interface{}) (interface{}, er | |||
| return value, nil | |||
| } | |||
|
|
|||
| // LongestPrefix is used to fetch the longest prefix match for the given | |||
| // constraints on the index. Note that this will not work with the memdb | |||
| // StringFieldIndex because it adds null terminators which prevent the | |||
There was a problem hiding this comment.
Would it be possible to make a call to PrefixFromArgs() to strip the \x00? Since this only supports prefix indexers, seems like that should be possible.
There was a problem hiding this comment.
@ryanuber I thought that, too. You can do that to strip the \x00 off the prefix, but when the LongestPrefix routine looks in the tree it hits the node above the actual entry in the index, so it doesn't report anything since that node isn't a leaf. Here's an example:
Say you have foo in your tree with a string field indexer. That'll be in there as foo\x00. When you do this algorithm for foobar it'll scan for the common prefix foo just fine and end up at a node with a prefix of foo and an edge to \x00 which is the proper leaf. It doesn't know that it should follow that edge, though, so it just says that there's no entry in there. Because we always put a suffix in there for the string indexer it always runs into this. If you knew to look up foo\x00 it would work, but you don't know what the longest prefix is when you do a query.
There was a problem hiding this comment.
I think the right fix would be to stop adding the \x00 for the string indexer and instead put that into the compound indexer, because that's where it really matters. That's a bigger change though, and doing that wouldn't affect this interface, just the cautionary comment :-)
|
@slackpad Left some comments and a question. Code looks great, just curious about the StringFieldIndex. See the question inline. |
| func (txn *Txn) LongestPrefix(table, index string, args ...interface{}) (interface{}, error) { | ||
| // Enforce that this only works on prefix indexes. | ||
| if !strings.HasSuffix(index, "_prefix") { | ||
| return nil, fmt.Errorf("index '%s' does not support prefix lookups", index) |
There was a problem hiding this comment.
Not sure I would say does not support prefix lookups because they could pass an index that supports it but didn't append "_prefix". Maybe just "must use prefix lookup on index"
There was a problem hiding this comment.
Good call - I'll change that.
|
LGTM |
Adds longest prefix matching for custom indexes.
This adds support for longest prefix matching as needed by hashicorp/consul#1764 (it's not yet integrated over there).
One unfortunate thing is that the null suffixes we add with the
StringFieldIndexdon't work with the algorithm as its currently written - it would need to see if there's a null edge and use that in order to terminate properly. TheLongestPrefixalgorithm is down in the immutable radix tree, so there's no super clean way to get it to understand this.For my application, I need a custom indexer, similar to the one in the unit test here that can index an empty string, and that will only get added for query templates, so this isn't a big deal. I commented
LongestPrefixwith details about this limitation, and I also made it look for common misconfigurations that will make it not work as expected.I think we could make under-the-hood improvements to remove this limitation with the
StringFieldIndexin the future, but this interface would still be useful (it's theLongestPrefixform ofFirst).