-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Json extract index filter support #12683
Json extract index filter support #12683
Conversation
@wirybeaver @Jackie-Jiang Split the original PR (https://github.com/apache/pinot/pull/12532/files) into two as discussed. Please take a look at this one for filter support once #12532 is merged. Ty! |
…o jsonExtractIndexFilterSupport
…o jsonExtractIndexFilterSupport
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12683 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2381 -55
Lines 133233 130795 -2438
Branches 20636 20267 -369
=============================================
- Hits 82274 0 -82274
- Misses 44911 130795 +85884
+ Partials 6048 0 -6048
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
return getMatchingFlattenedDocIds(filter, false); | ||
} | ||
|
||
private RoaringBitmap getMatchingFlattenedDocIds(FilterContext filter, boolean allowNestedExclusivePredicate) { |
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.
Trying to understand why we cannot always allow nested exclusive predicate?
for (int i = 1; i < numChildren; i++) { | ||
matchingDocIds.or(getMatchingFlattenedDocIds(children.get(i))); | ||
matchingDocIds.or(getMatchingFlattenedDocIds(children.get(i), allowNestedExclusivePredicate)); | ||
} | ||
return matchingDocIds; | ||
} | ||
case PREDICATE: { |
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.
Should we add NOT
since we allow exclusive predicate?
@@ -367,10 +379,18 @@ public void convertFlattenedDocIdsToDocIds(Map<String, RoaringBitmap> valueToFla | |||
} | |||
|
|||
@Override | |||
public Map<String, RoaringBitmap> getMatchingFlattenedDocsMap(String jsonPathKey) { | |||
public Map<String, RoaringBitmap> getMatchingFlattenedDocsMap(String jsonPathKey, @Nullable String filterString) { |
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.
Not introduced in this PR, but I think we should support json path key with and without $
prefix. See getMatchingFlattenedDocIds()
for reference
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 the support
cb36a1b
to
8e45c63
Compare
2581212
to
3771339
Compare
@Jackie-Jiang updated the PR to let As for the nested exclusive predicates, I think my understanding was incorrect. We can't infact support nested exclusive predicates (see https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java#L106). Turns out, the contract is for the predicates to maintain context, and therefore trying to solve nested exclusive predicates by flipping results of individual flattened doc id bitmaps would be wrong. Since our usecase does not require nested exclusive predicates, I removed that part entirely. Do have a look ty! |
This PR depends on #12532
Allows contextual filtering of documents when using
jsonExtractIndex
. EgjsonExtractIndex(jsonField, '$.arrField[*].f1', 'INT_ARRAY', '0', '"$.arrField[*].f2" > 2')
returns[3, 0]
i.e. Only return matching array elements that match the given json predicate.