-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for derive source feature and Implementing it for DateFieldMapper #17383
base: main
Are you sure you want to change the base?
Add support for derive source feature and Implementing it for DateFieldMapper #17383
Conversation
❌ Gradle check result for 5aa6a8c: 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? |
* @param docId - docId for which we want to derive the source | ||
* @throws IOException | ||
*/ | ||
public void buildDerivedSource(XContentBuilder builder, LeafReader leafReader, int docId) throws IOException { |
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.
Why do we need 2 separate methods i.e. buildDerivedSource
and deriveSource
?
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.
Removed the buildDerivedSource
method and embeded derive source feasibility checks under single method canDeriveSource
only.
server/src/main/java/org/opensearch/index/mapper/FieldMapper.java
Outdated
Show resolved
Hide resolved
boolean isStoredFieldPresent = false; | ||
// 1. Lookup stored field, which will help in preserving order of values in case of multi value field | ||
// 2. If field value is not found using stored field, lookup doc values | ||
if (mappedFieldType.isStored()) { |
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.
Since stored fields are stored in row fashion, it may turn out to be slower than doc values. Can we create an issue to benchmark what to prefer if both are available?
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 fetching few values, doc values would be faster for almost all cases as it can be served from OS cache(it can be very performant as multiple doc ids can be served from the same page) as well, whereas stored fields are stored in row fashion so for this serving as many doc ids as doc values might not be possible from a single memory page.
But while deriving source with multiple fields using stored field(considering for most fields, stored field is enabled) might be performant as for a single doc, all the field would be in nearby locality in memory. This we can benchmark, for now going with doc value first approach only considering wider cases where stored field would not be enabled explicitly in index field mapping and by default it's false.
server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java
Outdated
Show resolved
Hide resolved
* @param docId - docId for which we want to derive the source | ||
* @throws IOException | ||
*/ | ||
protected void deriveSource(XContentBuilder builder, LeafReader leafReader, int docId) throws IOException { |
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.
Will this support nested and/or object fields?
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.
Initially, will keep this feature disabled for nested object, later we will expand it for nested object fields as well.
@@ -226,6 +232,58 @@ private static DateFieldMapper toType(FieldMapper in) { | |||
return (DateFieldMapper) in; | |||
} | |||
|
|||
@Override | |||
protected void deriveSource(XContentBuilder builder, LeafReader leafReader, int docId) throws IOException { | |||
possibleToDeriveSource(); |
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 field level parameter that enables this?
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.
Yes, derivedSourceSupported
flag is defined in MappedFieldType
, which controls the feature at field level.
…DateFieldMapper Signed-off-by: Tanik Pansuriya <[email protected]>
Signed-off-by: Tanik Pansuriya <[email protected]>
…e method only Signed-off-by: Tanik Pansuriya <[email protected]>
5aa6a8c
to
ab3aa0a
Compare
❌ Gradle check result for ab3aa0a: 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? |
Signed-off-by: Tanik Pansuriya <[email protected]>
Signed-off-by: Tanik Pansuriya <[email protected]>
❌ Gradle check result for b0d4600: 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? |
Description
Adding interface for support of building source using stored/docvalues in FieldMapper
Added derivedSourceSupported flag to allowlist field mapper for this feature
Implemented these methods for DateFieldMapper
Tested the changes by integrating the flow with "doc by id" query path, cases covered:
Related Issues
Resolves #17073
Part of feature #9568
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.