Preserve percolator unmapped-field behavior after mapping updates

[officialasishkumar]

Description

This change preserves index.percolator.map_unmapped_fields_as_text across dynamic mapping updates.

The percolator field mapper was rebuilding merged field state with empty builder settings during mapping updates, which reset the percolator field\s### Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

Validation:
./gradlew :modules:percolator:test --tests 'org.opensearch.percolator.PercolatorQuerySearchTests.testMapUnmappedFieldAsText' --tests 'org.opensearch.percolator.PercolatorQuerySearchTests.testMapUnmappedFieldAsTextAfterDynamicMappingUpdate' --tests 'org.opensearch.percolator.PercolatorFieldMapperTests.testPercolatorFieldMapperUnMappedField'

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 146b541)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Fallback Logic The getMapUnmappedFieldAsText method falls back to mapUnmappedFieldsAsText only when indexSettings.isEmpty(). This condition may be too narrow — if index settings are non-empty but do not explicitly contain the percolator setting, the method will still call INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings), which returns the default value rather than the preserved value. This could silently reset the setting in some edge cases. private boolean getMapUnmappedFieldAsText(Settings indexSettings) { if (indexSettings.isEmpty()) { return mapUnmappedFieldsAsText; } return INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings); } Field Visibility The mapUnmappedFieldsAsText field in Builder is package-private (no access modifier). It is set directly from getMergeBuilder() in the outer class. Consider making it private and providing a setter, or at minimum documenting the intent, to avoid unintended external mutation. private boolean mapUnmappedFieldsAsText;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 146b541

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use setting existence check instead of emptiness check Using indexSettings.isEmpty() as the condition to fall back to the preserved value is fragile — non-empty settings that simply don't contain the percolator key will still call INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings) and return the setting's default value, ignoring the preserved mapUnmappedFieldsAsText. A more robust check would be to test whether the specific setting key is actually present in indexSettings. modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [185-190] private boolean getMapUnmappedFieldAsText(Settings indexSettings) { - if (indexSettings.isEmpty()) { + if (!INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.exists(indexSettings)) { return mapUnmappedFieldsAsText; } return INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that indexSettings.isEmpty() is a fragile check — if indexSettings is non-empty but doesn't contain the percolator key, the fallback to mapUnmappedFieldsAsText won't trigger. Using INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.exists(indexSettings) is more semantically correct and robust.	Medium
General	Set preserved value before builder initialization The field mapUnmappedFieldsAsText is set directly on the Builder after init(this), but init() may internally call build() or other methods that read mapUnmappedFieldsAsText before it is assigned. To ensure the value is always available during any initialization logic, set it before calling init(this). modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [128-131] Builder builder = new Builder(simpleName(), queryShardContext); +builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; builder.init(this); -builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; return builder; Suggestion importance[1-10]: 2 __ Why: The init() method in ParametrizedFieldMapper.Builder copies parameter values from the existing mapper and does not call build() or read mapUnmappedFieldsAsText, so the ordering concern is unlikely to be a real issue. The suggestion is speculative and offers minimal practical benefit.	Low

---

Previous suggestions

Suggestions up to commit 9c85dcf

Category	Suggestion	Impact
General	Use nullable type for safer fallback logic Using indexSettings.isEmpty() as the condition to fall back to the preserved value is fragile — an empty settings object could occur in other scenarios unrelated to mapping merges, and a non-empty settings object during a merge might still override the preserved value incorrectly. A more explicit approach would be to use a nullable Boolean for mapUnmappedFieldsAsText and only fall back when it has been explicitly set (i.e., non-null), making the intent clearer and the logic more robust. modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [185-190] +private Boolean mapUnmappedFieldsAsText = null; + private boolean getMapUnmappedFieldAsText(Settings indexSettings) { - if (indexSettings.isEmpty()) { + if (mapUnmappedFieldsAsText != null) { return mapUnmappedFieldsAsText; } return INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about using indexSettings.isEmpty() as a proxy for detecting merge scenarios, which could be fragile. However, the improved code changes the field declaration location and type in a way that doesn't cleanly map to the existing code structure, making the suggestion harder to apply directly.	Low
Possible issue	Ensure field assignment order is safe after init The field mapUnmappedFieldsAsText is being set directly on the Builder after init(this), but init() may internally call copyPropertiesFromSource or similar methods that could reset or overwrite fields. Verify that setting mapUnmappedFieldsAsText after init(this) is safe and won't be overwritten by any logic inside init. If init can overwrite it, the assignment should be done before calling init, or init should be overridden to handle this field. modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [128-131] Builder builder = new Builder(simpleName(), queryShardContext); +builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; builder.init(this); -builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; return builder; Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about whether init(this) could overwrite mapUnmappedFieldsAsText, but without evidence that init() actually resets this field, this is speculative. The reordering could be a valid precaution but may not be necessary.	Low

Suggestions up to commit 35f2b5d

Category	Suggestion	Impact
General	Use explicit null-check instead of empty settings check Using indexSettings.isEmpty() as the condition to fall back to the preserved value is fragile — an empty settings object could occur in other scenarios unrelated to mapping merges, and non-empty settings might still lack the specific percolator setting. A more robust approach would be to initialize mapUnmappedFieldsAsText with a sentinel value (e.g., null using Boolean) and only fall back when it hasn't been explicitly set from a prior mapper instance. modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [185-190] +private Boolean mapUnmappedFieldsAsText = null; + private boolean getMapUnmappedFieldAsText(Settings indexSettings) { - if (indexSettings.isEmpty()) { + if (mapUnmappedFieldsAsText != null && !indexSettings.hasValue(INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.getKey())) { return mapUnmappedFieldsAsText; } return INDEX_MAP_UNMAPPED_FIELDS_AS_TEXT_SETTING.get(indexSettings); } Suggestion importance[1-10]: 5 __ Why: The concern about indexSettings.isEmpty() being fragile is valid — it's a heuristic that could fail in edge cases. However, the improved code changes the field type from boolean to Boolean and introduces a different condition using hasValue(), which is a more targeted check. The suggestion has merit but involves a more significant refactor than necessary.	Low
Possible issue	Set preserved field before calling init The field mapUnmappedFieldsAsText is being set directly on the Builder after init(this) is called, but init() may internally call build() or other methods that could read mapUnmappedFieldsAsText before it is set. Ensure the field is set before any method that might depend on it, or set it before calling init(this). modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java [128-131] Builder builder = new Builder(simpleName(), queryShardContext); +builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; builder.init(this); -builder.mapUnmappedFieldsAsText = mapUnmappedFieldsAsText; return builder; Suggestion importance[1-10]: 3 __ Why: The concern about init() potentially reading mapUnmappedFieldsAsText before it's set is speculative. Looking at the typical ParametrizedFieldMapper.Builder.init() implementation, it copies parameters from an existing mapper but doesn't call build() or read mapUnmappedFieldsAsText. The suggestion is a minor precautionary reordering with low practical impact.	Low

[github-actions]

❌ Gradle check result for 35f2b5d: 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?

[github-actions]

Persistent review updated to latest commit 9c85dcf

[github-actions]

❌ Gradle check result for 9c85dcf: 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?

[officialasishkumar]

Investigated the gradle-check failures for both builds (73967 and 73972). The Jenkins build logs are not publicly accessible, so I verified locally:

• ./gradlew :modules:percolator:compileJava — compiles successfully
• ./gradlew :modules:percolator:compileTestJava — compiles successfully
• ./gradlew :modules:percolator:test — all tests pass (including the new testMapUnmappedFieldAsTextAfterDynamicMappingUpdate)
• ./gradlew :modules:percolator:internalClusterTest — all tests pass
• ./gradlew :modules:percolator:precommit — all checks pass (spotless, forbiddenApis, licenseHeaders, jarHell, testingConventions, etc.)

The CI failures appear to be caused by flaky tests unrelated to the percolator changes. Other recent PRs on the same base branch are passing gradle-check without issues, so this is likely a transient test failure in the broader test suite.

Retriggering CI to confirm.

[github-actions]

Persistent review updated to latest commit 146b541

[github-actions]

✅ Gradle check result for 146b541: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (d26ad17) to head (146b541).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21124      +/-   ##
============================================
- Coverage     73.35%   73.31%   -0.05%     
+ Complexity    73209    73195      -14     
============================================
  Files          5921     5921              
  Lines        333798   333803       +5     
  Branches      48124    48125       +1     
============================================
- Hits         244862   244723     -139     
- Misses        69387    69494     +107     
- Partials      19549    19586      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.