Add support for deriving source field from docValues in FieldMapper #17040
Conversation
rayshrey
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
|
❌ Gradle check result for a5057b4: 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? |
|
|
||
| // generic implementation, override in subclasses for specific implementation | ||
| protected Object deriveSource(LeafReader leafReader, int docId) throws IOException { | ||
| return null; |
There was a problem hiding this comment.
Throw an exception here instead
| @@ -800,4 +817,18 @@ public boolean getIgnoreMalformed() { | |||
| public Long getNullValue() { | |||
| return nullValue; | |||
| } | |||
|
|
|||
| @Override | |||
| protected String[] deriveSource(LeafReader leafReader, int docId) throws IOException { | |||
There was a problem hiding this comment.
how would we ensure that the entity creating the source can understand if it should use an array or just a single string while trying to serialize the source
There was a problem hiding this comment.
I was thinking if we can let the mapper method itself be responsible to create the source
There was a problem hiding this comment.
Modified the function signature to include a XContentBuilder - it will be the responsibility of each mapper to add data to this builder, this will allow the mapper to handle cases for single value or array in the mapper itself
|
|
||
| @Override | ||
| protected String[] deriveSource(LeafReader leafReader, int docId) throws IOException { | ||
| SortedNumericDocValues sortedNumericDocValues = leafReader.getSortedNumericDocValues(name()); |
There was a problem hiding this comment.
What is doc values are not available for the field? Can we have generic validations/assertions around expected data structures as well to ensure that if this method is called, we have a way to create source
There was a problem hiding this comment.
Say if a field is stored, should we also allow to use that even if DVs are not present
There was a problem hiding this comment.
Added generic validations in the latest revision
Say if a field is stored, should we also allow to use that even if DVs are not present
Yes we can do that but for the current PR I think we should limit it to docValues only, will add support for stored fields in subsequent PRs
Signed-off-by: Shreyansh Ray <[email protected]>
Signed-off-by: Shreyansh Ray <[email protected]>
Signed-off-by: Shreyansh Ray <[email protected]>
rayshrey
force-pushed
the
derived-source-interface
branch
from
a5057b4 to
e16b768
Compare
|
❕ Gradle check result for e16b768: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17040 +/- ##
============================================
- Coverage 72.31% 72.26% -0.06%
- Complexity 65346 65349 +3
============================================
Files 5301 5301
Lines 303805 303833 +28
Branches 44030 44034 +4
============================================
- Hits 219702 219559 -143
- Misses 66055 66257 +202
+ Partials 18048 18017 -31 ☔ View full report in Codecov by Sentry. |
|
@rayshrey is this feature planned to be released in 2.19 version of Opensearch? |
Description
Add support for deriving source field from docValues in FieldMapper
Following changes have been done:
Related Issues
#17073
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.