Updating the FieldMappers to insert call addField of DocumentInput#21001
Updating the FieldMappers to insert call addField of DocumentInput#21001darjisagar7 wants to merge 3 commits intoopensearch-project:mainfrom
Conversation
|
Failed to generate code suggestions for PR |
|
❌ Gradle check result for 7f26163: 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? |
7f26163 to
80f72cd
Compare
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 78ffe1a.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
Failed to generate code suggestions for PR |
80f72cd to
93426fa
Compare
|
Failed to generate code suggestions for PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #21001 +/- ##
============================================
- Coverage 73.35% 73.31% -0.05%
- Complexity 73209 73303 +94
============================================
Files 5921 5921
Lines 333798 333979 +181
Branches 48124 48155 +31
============================================
- Hits 244862 244850 -12
- Misses 69387 69564 +177
- Partials 19549 19565 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
78ffe1a to
bff52e7
Compare
|
Persistent review updated to latest commit bff52e7 |
bff52e7 to
6ce9abf
Compare
|
Persistent review updated to latest commit 6ce9abf |
|
❌ Gradle check result for 6ce9abf: 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? |
|
Persistent review updated to latest commit 6ce9abf |
| @Override | ||
| protected void parseCreateFieldForPluggableFormat(ParseContext context) throws IOException { | ||
| final String value = context.externalValueSet() ? context.externalValue().toString() : context.parser().textOrNull(); | ||
| if (value == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Maybe you could further extract the common logic
private String extractValue(ParseContext context) throws IOException {
String value = context.externalValueSet()
? context.externalValue().toString()
: context.parser().textOrNull();
return value;
}
| protected final boolean isPluggableDataFormatFeatureEnabled(ParseContext parseContext) { | ||
| return isPluggableDataFormatEnabled(parseContext.indexSettings().getSettings()); | ||
| } | ||
|
|
There was a problem hiding this comment.
We can cache this instead of reading from index settings everytime?
| protected MultiFields multiFields; | ||
| protected CopyTo copyTo; | ||
| protected DerivedFieldGenerator derivedFieldGenerator; | ||
| private final AtomicBoolean isPluggableDataFormatFeatureEnabled = new AtomicBoolean(false); |
There was a problem hiding this comment.
This seems unused, prefer using volatile once you implement the caching logic
| assert parser.currentToken() == XContentParser.Token.START_OBJECT; | ||
| parser.nextToken(); // Skip the outer START_OBJECT. Need to return on END_OBJECT. | ||
|
|
||
| LinkedList<String> path = new LinkedList<>(Collections.singleton(fieldType().name())); | ||
| HashSet<String> pathParts = new HashSet<>(); | ||
| while (parser.currentToken() != XContentParser.Token.END_OBJECT) { | ||
| parseToken(parser, context, path, pathParts); | ||
| } | ||
|
|
There was a problem hiding this comment.
This logic is again redundant
| /** | ||
| * Gates the functionality of pluggable dataformat feature. | ||
| */ | ||
| public static final String PLUGGABLE_DATAFORMAT_EXPERIMENTAL_FLAG = FEATURE_FLAG_PREFIX + "pluggable.dataformat.enabled"; | ||
|
|
||
| public static final Setting<Boolean> PLUGGABLE_DATAFORMAT_EXPERIMENTAL_SETTING = Setting.boolSetting( | ||
| PLUGGABLE_DATAFORMAT_EXPERIMENTAL_FLAG, | ||
| false, | ||
| Property.NodeScope | ||
| ); | ||
|
|
There was a problem hiding this comment.
Are we gating behind both feature flag and index settings?
1. Introducing FeatureFlag and Index Setting for pluggable dataformat feature. 2. Updating the FieldMappers to insert fields in DocumentInput for Multi Format Engine Signed-off-by: Sagar Darji <darsaga@amazon.com> # Conflicts: # CHANGELOG.md
…Mapper class Signed-off-by: Sagar Darji <darsaga@amazon.com>
6ce9abf to
2290cfc
Compare
|
Persistent review updated to latest commit 2290cfc |
|
❌ Gradle check result for 2290cfc: 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? |
|
Persistent review updated to latest commit 2290cfc |
|
❕ Gradle check result for 2290cfc: 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. |
|
Persistent review updated to latest commit e74c078 |
|
|
||
| @Override | ||
| protected void parseCreateFieldForPluggableFormat(ParseContext context) throws IOException { | ||
| // TODO check how we can support addWithKey for pluggable dataformat |
There was a problem hiding this comment.
We should throw here if it is not supported?
There was a problem hiding this comment.
Agree, I can throw an exception here
| * @throws UnsupportedOperationException if the mapper does not support pluggable data formats | ||
| */ | ||
| @ExperimentalApi | ||
| protected abstract void parseCreateFieldForPluggableFormat(ParseContext context) throws IOException; |
There was a problem hiding this comment.
We can't have this abstract as this breaks everything that implements FieldMapper outside of the plugin. This should throw unsuspported?
There was a problem hiding this comment.
With pluggable dataformat if lucene is selected then every functionality that is working today should work as it is so adding the abstract here ensures that this function is Overriden at all the FieldMapper places.
| context.documentInput().addField(fieldType(), value); | ||
| } | ||
|
|
||
| private String parseKeywordValue(ParseContext context) throws IOException { |
There was a problem hiding this comment.
nice! how are we testing we are testing all such common functionalities continue to work? I see have a CapturingDocumentInput which is being run against the tests written, but how do we ensure that existing tests can also work there?
There was a problem hiding this comment.
The current tests are working as it is we have not modified them, so that should give us the confidence that existing flow is working as expected. Or do we need some other mechanism to confirm this?
Description
This PR adds the following functionalities:
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.