Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indexes search for exact match rather than submatches #232

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

Hydrocharged
Copy link
Contributor

No description provided.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This is definitely a bug, but this is not the right fix. The sublist logic is just plain wrong. This code is really old, left over from before my rewrite of indexes. I think it's basically tailor made for pilosa indexes, but most database indexes don't work like that: if partial matching is supported, the order of the keys matters.

You should enforce an exact match, not a subset match. And there should be a couple unit tests of this behavior.

@Hydrocharged Hydrocharged changed the title Indexes search for exact match first before submatches Indexes search for exact match rather than submatches Nov 11, 2020
@Hydrocharged Hydrocharged requested review from zachmu and removed request for zachmu November 11, 2020 00:35
@Hydrocharged
Copy link
Contributor Author

Switching the order of the two indexes in indexes_test.go was enough to trigger the issue that is being fixed in existing tests (which were updated), and I added another (simpler) test into the mix just as a verification.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!

@Hydrocharged Hydrocharged merged commit 117786c into master Nov 11, 2020
@Hydrocharged Hydrocharged deleted the daylon/index-priority branch November 11, 2020 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants