-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Support object fields in star-tree index #16728
base: main
Are you sure you want to change the base?
Support object fields in star-tree index #16728
Conversation
Signed-off-by: bharath-techie <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16728 +/- ##
============================================
- Coverage 72.21% 72.12% -0.09%
+ Complexity 65335 65315 -20
============================================
Files 5318 5318
Lines 304081 304119 +38
Branches 43995 44005 +10
============================================
- Hits 219578 219360 -218
- Misses 66541 66765 +224
- Partials 17962 17994 +32 ☔ View full report in Codecov by Sentry. |
@@ -690,6 +710,10 @@ public boolean isFieldPartOfCompositeIndex(String field) { | |||
return fieldsPartOfCompositeMappings.contains(field); | |||
} | |||
|
|||
public boolean isCompositeIndexFieldNestedField(String field) { |
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.
Is there a use-case where this method can be used separately from the isFieldPartOfCompositeIndex
? Otherwise, can we combine both methods?
Signed-off-by: bharath-techie <[email protected]>
|
if (context.indexSettings().isCompositeIndex() && context.mapperService().isFieldPartOfCompositeIndex(arrayFieldName)) { | ||
if (context.indexSettings().isCompositeIndex() | ||
&& (context.mapperService().isFieldPartOfCompositeIndex(arrayFieldName) | ||
|| context.mapperService().isFieldPartOfCompositeIndex(context.path().pathAsText(arrayFieldName)))) { |
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.
Do we have a test for this new check?
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.
We have tests for the new checks in IT. But I found an edge case where its failing.
{ "index" : { "_index" : "logs-2419985" } }
{"@timestamp": 1706431663, "clientip": "215.79.163.253",
"request": "GET /spanish/index.html HTTP/1.0",
"nested": {"status": [218,219]}, "size": 202, "status": 218}
Say status
is part of star-tree mapping but nested.status
has array as above example.
arrayFieldName = "status"
context.path().pathAsText(arrayFieldName)) = "nested.status"
so the condition will satisfy for arrayFieldName and doc addition will be blocked.
Changes look good, please add changelog entry. |
❌ Gradle check result for 995f912: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
995f912
to
3897a5e
Compare
Signed-off-by: bharath-techie <[email protected]>
3897a5e
to
4da6d64
Compare
Description
We'll support object type fields with this change for star-tree index.
Star-tree index will still block array values for the object fields.
Example :
Related Issues
#16730
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.