-
Notifications
You must be signed in to change notification settings - Fork 114
Lucene: index filters support #3688
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
base: main
Are you sure you want to change the base?
Conversation
Add support for index filters by predicates or by IndexMaintenanceFilter. However, Lucene can only support ALL or NONE. Filtering SOME will cause an exception. Resolves FoundationDB#3065
| .setSerializer(TextIndexTestUtils.COMPRESSING_SERIALIZER); | ||
| if (filterOut) { | ||
| recordStore = builder | ||
| .setIndexMaintenanceFilter((i, r) -> IndexMaintenanceFilter.IndexValues.NONE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably more absolute than we want.
It seems valuable to have this be a filter than is conditional in some way on the record. i.e. some field is even, or if it is equal to (or not equal to) a specific value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests in LuceneScanAllEntriesTest.
| .setSerializer(TextIndexTestUtils.COMPRESSING_SERIALIZER); | ||
| if (filterOut) { | ||
| recordStore = builder | ||
| .setIndexMaintenanceFilter((i, r) -> IndexMaintenanceFilter.IndexValues.NONE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems worthwhile to test predicate support too, perhaps parameterizing this method, and the eventual test for whether to filter with a predicate or a filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
√
| for (int i = 0; i < iLast; i++) { | ||
| recordStore.saveRecord(multiEntryMapDoc(77L * i, ENGINEER_JOKE + iLast, group)); | ||
| } | ||
| final Set<Index> indexSet = recordStore.getIndexDeferredMaintenanceControl().getMergeRequiredIndexes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually assert that anything was filtered out, just that Lucene didn't connect to the deferred maintenance control. It seems better to be testing the actual desired behavior, which is that if you filter something out, it doesn't show up when you scan the index.
LuceneIndexTestDataModel may be helpful here, by removing everything from the data model that should be filtered out before validating that the index is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
√
| int groupingCount = 1; | ||
| final int groupedCount = 4 - groupingCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two could probably be inlined, although that might become moot if you use LuceneIndexTestDataModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
√
| } | ||
|
|
||
| @Nullable | ||
| private <M extends Message> FDBIndexableRecord<M> maybeFilterRecord(FDBIndexableRecord<M> record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rec I believe is what I have seen in most places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
√
| assertNotEquals(needMerge, filterOut); | ||
| } | ||
|
|
||
| private TestRecordsTextProto.MapDocument multiEntryMapDoc(long id, String text, int group) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test that fails:
- filter returns
SOME - filter throws exception
Add support for index filters by predicates or by IndexMaintenanceFilter.
However, Lucene can only support ALL or NONE. Filtering SOME will cause an exception.
Resolves #3065