Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts

[ahkcs]

Description

On the Calcite PPL path, an aggregation grouped on a field that is mapped keyword in some indices of a wildcard pattern and text in others cannot use native pushdown. The multi-index type merge collapses the field to text-without-.keyword, which has no doc values, so the aggregation runs as a per-document _source script over every document — correct, but a full-index scan that is orders of magnitude slower on a wide pattern.

This PR adds an opt-in mode that returns a fast, partial answer instead: it aggregates over only the subset of indices where the field is natively aggregatable (keyword) and attaches a warning naming the ones it excluded.

So the choice becomes complete-but-slow (default) vs fast-but-partial (opt-in) — both correct, differing in coverage and speed.

How it works

1. Warning channel. Successful PPL JSON responses gain an optional warnings: [{type, message, detail}] array, emitted only when non-empty (existing responses are byte-for-byte unchanged):

   "warnings": [{
     "type": "PARTIAL_RESULT",
     "message": "Results exclude 1 of 2 indices due to a text/keyword mapping conflict on [applicationid].",
     "detail": "[applicationid] is not mapped as keyword in every queried index, so these indices were excluded from the aggregation: [logs-text]. Map [applicationid] as keyword across all indices to include them."
   }]

2. Partial-result plan (PartialResultAggregatePushdown). When the mode is on and the group key is a text/keyword conflict, the scan is narrowed to the aggregatable index subset and the aggregation pushed down over just that subset (size = 0, no PIT). The partitioning logic is unit-tested in isolation.
3. Per-request override. A partial_result boolean in the query body (mirroring profile) overrides the cluster setting for one query; absent → cluster setting decides.

Behavior

Cluster setting plugins.query.partial_result.on_mapping_conflict.enabled (default false):

Query	Off (default)	On
stats count() by <conflict field>	complete result, slow (_source scan of all docs)	fast result over the keyword subset + PARTIAL_RESULT warning
no-conflict / single-index aggregation	complete, no warning	complete, no warning (unchanged)
any of the above with format=csv	as above	falls through to the complete result (CSV has no warning channel)

Key points

• Opt-in, default off. A partial result is knowingly incomplete, so it never happens silently; with the setting off the change is behavior-preserving.
• Never degrades silently. Only the JSON shape carries warnings, so CSV/RAW/VIZ fall through to the complete result rather than dropping data unannounced.
• Deterministic selection. Keep the keyword group whenever one exists; otherwise the text-with-.keyword group; always exclude bare text. The result never depends on how many indices of each type match.
• Calcite PPL path only; V2/legacy untouched.

Not in scope: recovering an excluded but aggregatable group (text-with-.keyword alongside a keyword group) — that needs a per-group split-and-union, a larger separate change. This is why the warning recommends mapping the field as keyword everywhere.

Related Issues

Check List

• New functionality includes testing (unit + integration).
• New functionality has been documented (docs/user/admin/settings.rst).
• New functionality has javadoc added.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 11b57b2)

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 Possible Issue The merge logic is commented out but the deep-copy call is added unconditionally. If indexMappings.size() > 1, the loop iterates over all mappings but never merges them—fieldTypes remains empty. The aggregation then sees no fields and cannot push down. This breaks multi-index queries when the cluster setting is off (the default). The removed MergeRuleHelper.merge call must be restored inside the loop. } else { // Merge deep copies: MergeRuleHelper mutates the field mappings in place, and the per-index // mappings retained above must stay intact for partial-result partitioning. for (IndexMapping indexMapping : indexMappings.values()) { MergeRuleHelper.merge(fieldTypes, deepCopy(indexMapping.getFieldMappings())); } } Possible Issue When allowPartialFallback is false (the recursive call from the partial path), resolvePartitionFields is still invoked and may return null for a constant group key. The null is then passed to tryPartialResultAggregate, which calls plan(partitionFields, ...) without a null check. If partitionFields is null, plan will throw a NullPointerException when it tries to call bucketNames.isEmpty() at line 64 of PartialResultAggregatePushdown. The code should skip the partial attempt entirely when partitionFields is null, or plan should handle null input. if (allowPartialFallback) { List<String> partitionFields = resolvePartitionFields(aggregate, project); if (partitionFields != null) { AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields); if (partial != null) { return partial; } } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 11b57b2

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Reset profile flag on cleanup The clearRequestScopedState() method should also reset the profile flag by calling QueryContext.setProfile(false). Without this, a profiled request's flag could leak to the next query on the same pooled thread. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [405-408] private static void clearRequestScopedState() { QueryProfiling.clear(); QueryContext.setPartialResultOverride(null); QueryContext.setWarningsSupported(false); + QueryContext.setProfile(false); } Suggestion importance[1-10]: 7 __ Why: Valid concern about thread-local state leakage. The profile flag is set via QueryContext.setProfile() at line 182 and should be cleared to prevent it from affecting subsequent queries on pooled threads, just like partialResultOverride and warningsSupported.	Medium
Possible issue	Add null checks for parameters Add explicit null checks for both bucketNames and mappings parameters at the start of the plan() method. This prevents potential NullPointerException if either parameter is unexpectedly null. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [63-65] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { - if (bucketNames.isEmpty() || mappings.size() < 2) { + if (bucketNames == null || bucketNames.isEmpty() || mappings == null || mappings.size() < 2) { return null; } Suggestion importance[1-10]: 4 __ Why: Adding null checks for bucketNames and mappings is defensive programming, but the callers in this codebase control these parameters and the existing isEmpty() and size() checks would throw NullPointerException if null, making issues immediately visible. The improvement is marginal.	Low
	Validate mappings before planning Add a null/empty check for mappings before passing it to PartialResultAggregatePushdown.plan(). If getIndexMappings() returns null or an empty map, the subsequent planning logic could fail unexpectedly. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [523-527] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project, List<String> partitionFields) { if (!QueryContext.isPartialResultEnabled(osIndex.getSettings())) { return null; } // A format with no warnings channel (CSV/RAW/VIZ) must not silently drop indices. if (!QueryContext.isWarningsSupported()) { return null; } try { Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); + if (mappings == null || mappings.isEmpty()) { + return null; + } PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(partitionFields, mappings); if (plan == null) { return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion to check for null/empty mappings is reasonable defensive programming, but getIndexMappings() is designed to return a non-null map (it returns Map.of() when empty). The subsequent plan() method already handles the empty case by checking mappings.size() < 2, so this adds minimal value.	Low

---

Previous suggestions

Suggestions up to commit cbf5074

Category	Suggestion	Impact
Possible issue	Prevent thread-local override leakage The method reads from ThreadContext and then falls back to the cluster setting. However, if the thread context is not properly cleared between requests (e.g., in a pooled thread scenario), a stale override from a previous request could leak into the current one. Verify that setPartialResultOverride(null) is always called in cleanup paths (e.g., in clearRequestScopedState) to prevent cross-request contamination. common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java [134-140] public static boolean isPartialResultEnabled(Settings settings) { String override = ThreadContext.get(PARTIAL_RESULT_OVERRIDE_KEY); if (override != null) { return Boolean.parseBoolean(override); } return settings.getSettingValue(Settings.Key.PARTIAL_RESULT_ON_MAPPING_CONFLICT); } +// Ensure clearRequestScopedState() in TransportPPLQueryAction always calls: +// QueryContext.setPartialResultOverride(null); Suggestion importance[1-10]: 7 __ Why: The concern about thread-local leakage is valid and important for correctness in pooled thread scenarios. However, the PR already addresses this in TransportPPLQueryAction.clearRequestScopedState() (lines 405-408), which calls setPartialResultOverride(null). The suggestion correctly identifies a critical pattern but the fix is already present.	Medium
General	Optimize repeated mapping flattening The traverseAndFlatten call is invoked for every index in the loop, which can be expensive for large wildcard patterns. If the mapping structure is complex or the number of indices is high, this repeated flattening could become a performance bottleneck. Consider caching the flattened mappings or moving the flattening outside the loop if the same mapping is reused. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [71-86] +Map<String, Map<String, OpenSearchDataType>> flatMappings = new LinkedHashMap<>(); for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { - Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); - switch (resolveBucketMapping(flatMapping, bucketNames)) { + flatMappings.put(entry.getKey(), OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings())); +} +for (Map.Entry<String, Map<String, OpenSearchDataType>> entry : flatMappings.entrySet()) { + switch (resolveBucketMapping(entry.getValue(), bucketNames)) { case KEYWORD -> keywordIndices.add(entry.getKey()); case TEXT_WITH_KEYWORD -> textKeywordIndices.add(entry.getKey()); case CONFLICTING_TYPE -> { return null; } default -> excludedIndices.add(entry.getKey()); } } Suggestion importance[1-10]: 5 __ Why: Pre-computing flattened mappings could improve performance for large wildcard patterns, but the impact depends on usage patterns. The suggestion is valid but not critical, as traverseAndFlatten is already optimized for typical cases.	Low
	Profile deep copy performance The deepCopy call is invoked for every index mapping in the loop to prevent MergeRuleHelper.merge from mutating the original mappings. However, if the number of indices or the depth of the mapping tree is large, this repeated deep copying could be expensive. Consider profiling this path under high-cardinality wildcard patterns to ensure it does not become a bottleneck. opensearch/src/main/java/org/opensearch/sql/opensearch/request/system/OpenSearchDescribeIndexRequest.java [120-124] } else { for (IndexMapping indexMapping : indexMappings.values()) { MergeRuleHelper.merge(fieldTypes, deepCopy(indexMapping.getFieldMappings())); } } +// Add a comment or log warning if indexMappings.size() is very large: +// if (indexMappings.size() > THRESHOLD) { LOG.warn("Deep copying {} mappings may be expensive", indexMappings.size()); } Suggestion importance[1-10]: 4 __ Why: The suggestion to profile or log warnings for large mapping sets is reasonable for observability, but the deep copy is necessary to prevent mutation (as documented in the test at lines 45-66). The performance concern is valid but speculative without evidence of actual bottlenecks.	Low
	Document constant group key bypass The partial-result path is attempted before the normal aggregation analysis, but if tryPartialResultAggregate returns null (e.g., no conflict or partial mode disabled), the code continues to the normal path. However, if resolvePartitionFields returns null (indicating a constant group key), the partial path is skipped entirely. Consider logging or documenting this early-exit behavior to clarify that constant group keys intentionally bypass partial-result logic. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [421-427] List<String> partitionFields = resolvePartitionFields(aggregate, project); if (partitionFields != null) { AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields); if (partial != null) { return partial; } +} else if (LOG.isDebugEnabled()) { + LOG.debug("Skipping partial-result path: group key is a constant with no field to partition on"); } Suggestion importance[1-10]: 3 __ Why: Adding a debug log for the constant group key case improves observability but has minimal impact. The behavior is already documented in resolvePartitionFields javadoc, so this is a minor enhancement.	Low

Suggestions up to commit ad3c0a5

Category	Suggestion	Impact
Possible issue	Guard against null settings parameter The method retrieves a cluster setting without verifying that settings is non-null. If settings is null when no override is present, this will throw a NullPointerException. Add a null check for settings to ensure robustness. common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java [134-140] public static boolean isPartialResultEnabled(Settings settings) { String override = ThreadContext.get(PARTIAL_RESULT_OVERRIDE_KEY); if (override != null) { return Boolean.parseBoolean(override); } + if (settings == null) { + return false; + } return settings.getSettingValue(Settings.Key.PARTIAL_RESULT_ON_MAPPING_CONFLICT); } Suggestion importance[1-10]: 5 __ Why: The suggestion adds a null check for the settings parameter. While callers in the codebase always provide a valid Settings instance, the defensive check prevents a potential NullPointerException and makes the method more robust against misuse.	Low
	Handle null field mappings safely The method does not handle the case where entry.getValue().getFieldMappings() returns null. If an index has no field mappings, traverseAndFlatten may throw a NullPointerException. Add a null check before flattening to prevent crashes. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [70-85] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { if (bucketNames.isEmpty() || mappings.size() < 2) { return null; } List<String> keywordIndices = new ArrayList<>(); List<String> textKeywordIndices = new ArrayList<>(); List<String> excludedIndices = new ArrayList<>(); for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { + Map<String, OpenSearchDataType> fieldMappings = entry.getValue().getFieldMappings(); + if (fieldMappings == null) { + excludedIndices.add(entry.getKey()); + continue; + } Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); + OpenSearchDataType.traverseAndFlatten(fieldMappings); switch (resolveBucketMapping(flatMapping, bucketNames)) { case KEYWORD -> keywordIndices.add(entry.getKey()); case TEXT_WITH_KEYWORD -> textKeywordIndices.add(entry.getKey()); case CONFLICTING_TYPE -> { return null; } default -> excludedIndices.add(entry.getKey()); } } ... Suggestion importance[1-10]: 4 __ Why: The suggestion adds a null check for fieldMappings before flattening. While IndexMapping is constructed with a non-null map in practice, the defensive check improves robustness against future changes or edge cases where an index might have no mappings.	Low
General	Validate mappings before planning The method retrieves index mappings and creates a plan without validating that the mappings are non-empty. If getIndexMappings() returns an empty map due to a transient error or misconfiguration, the plan logic may behave unexpectedly. Add a defensive check to ensure mappings are present before proceeding. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [471-474] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project, List<String> bucketNames) { if (!QueryContext.isPartialResultEnabled(osIndex.getSettings())) { return null; } if (!QueryContext.isWarningsSupported()) { return null; } try { Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); + if (mappings == null || mappings.isEmpty()) { + return null; + } PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } ... Suggestion importance[1-10]: 3 __ Why: The suggestion adds a defensive null/empty check for mappings, but getIndexMappings() is guaranteed to return a non-null map (it returns Map.of() when uninitialized). The empty-map case is already handled by plan()'s mappings.size() < 2 guard, so this check is redundant.	Low
	Validate kept indices before narrowing The code constructs a narrowed index name by joining plan.keptIndices() with a comma, but does not verify that the list is non-empty. If keptIndices() is empty, the resulting index name will be an empty string, which may cause downstream errors. Validate that the list is not empty before constructing the index. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [478-494] +if (plan.keptIndices().isEmpty()) { + return null; +} OpenSearchIndex narrowedIndex = new OpenSearchIndex( osIndex.getClient(), osIndex.getSettings(), String.join(",", plan.keptIndices())); CalciteLogicalIndexScan narrowedScan = new CalciteLogicalIndexScan( getCluster(), traitSet, hints, table, narrowedIndex, getRowType(), pushDownContext.cloneWithOsIndex(narrowedIndex)); AbstractRelNode pushed = narrowedScan.pushDownAggregate(aggregate, project, false); if (pushed == null) { return null; } Suggestion importance[1-10]: 2 __ Why: The suggestion checks if keptIndices() is empty before constructing the narrowed index. However, the plan() method already ensures keptIndices is non-empty (it returns null when no aggregatable subset exists), so this check is redundant and adds unnecessary code.	Low

Suggestions up to commit c06668d

Category	Suggestion	Impact
General	Clear request ID to prevent leakage The method clears request-scoped state but does not clear the request ID set by QueryContext.addRequestId(). This could cause request ID leakage across pooled threads, where subsequent queries inherit the previous request's ID, leading to incorrect tracing and logging correlation. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [409-413] private static void clearRequestScopedState() { QueryProfiling.clear(); QueryContext.setPartialResultOverride(null); QueryContext.setWarningsSupported(false); + QueryContext.clearRequestId(); } Suggestion importance[1-10]: 7 __ Why: This identifies a potential issue where QueryContext.addRequestId() is called but never cleared, which could cause request ID leakage across pooled threads. However, the suggestion assumes clearRequestId() exists without verifying it in the codebase.	Medium
	Improve exception handling visibility The method catches all exceptions and returns null, which silently suppresses errors during partial-result planning. This could hide critical issues like network failures or mapping resolution errors. Consider logging at a higher level (WARN) or re-throwing specific exceptions that indicate system failures rather than expected planning failures. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [456-506] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project, List<String> bucketNames) { if (!QueryContext.isPartialResultEnabled(osIndex.getSettings())) { return null; } - // A format with no warnings channel (CSV/RAW/VIZ) must not silently drop indices. if (!QueryContext.isWarningsSupported()) { return null; } try { Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } OpenSearchIndex narrowedIndex = new OpenSearchIndex( osIndex.getClient(), osIndex.getSettings(), String.join(",", plan.keptIndices())); CalciteLogicalIndexScan narrowedScan = new CalciteLogicalIndexScan( getCluster(), traitSet, hints, table, narrowedIndex, getRowType(), pushDownContext.cloneWithOsIndex(narrowedIndex)); - // allowPartialFallback=false: the subset is already narrowed, so keep this one-shot. AbstractRelNode pushed = narrowedScan.pushDownAggregate(aggregate, project, false); if (pushed == null) { - return null; // narrowed subset still can't push down -> leave un-pushed + return null; } CalcitePlanContext.addWarning(plan.warning()); return pushed; } catch (Exception e) { - if (LOG.isDebugEnabled()) { - LOG.debug("Cannot apply partial-result aggregate pushdown for {}", aggregate, e); - } + LOG.warn("Failed to apply partial-result aggregate pushdown for {}", aggregate, e); return null; } } Suggestion importance[1-10]: 5 __ Why: Changing from DEBUG to WARN logging could be useful for visibility, but the current DEBUG level is appropriate for a planning fallback that is expected to fail in certain scenarios. The suggestion doesn't address a bug, just a logging preference.	Low
	Cache flattened mappings for performance The method flattens the mapping for every index in the loop, which could be expensive for large wildcard patterns with many indices. Consider caching the flattened mappings or performing the flattening once per index mapping when it is first retrieved, rather than repeating it for each planning call. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [60-78] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { if (bucketNames.isEmpty() || mappings.size() < 2) { return null; } List<String> keywordIndices = new ArrayList<>(); List<String> textKeywordIndices = new ArrayList<>(); List<String> excludedIndices = new ArrayList<>(); + Map<String, Map<String, OpenSearchDataType>> flatMappingsCache = new HashMap<>(); + for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { - // Flatten so a nested object field (mapping tree resource -> attributes -> applicationid) is - // keyed by its dotted path, matching the bucket field name Calcite resolved. - Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); + Map<String, OpenSearchDataType> flatMapping = flatMappingsCache.computeIfAbsent( + entry.getKey(), + k -> OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings())); switch (resolveBucketMapping(flatMapping, bucketNames)) { case KEYWORD -> keywordIndices.add(entry.getKey()); case TEXT_WITH_KEYWORD -> textKeywordIndices.add(entry.getKey()); default -> excludedIndices.add(entry.getKey()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion proposes caching flattened mappings within the plan method, but the cache is local to a single invocation and provides no benefit. The flattening happens once per index per call, which is already optimal for this method's scope.	Low

Suggestions up to commit 0c2ee57

Category	Suggestion	Impact
Possible issue	Prevent NPE on null mapping type The method calls type.getMappingType() without null-checking the result. If getMappingType() returns null (e.g., for a malformed or unsupported type), the subsequent enum comparison will throw a NullPointerException. Add a null check before comparing to MappingType.Keyword to handle unexpected type states gracefully. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [107-124] static MappingResolution resolveBucketMapping( Map<String, OpenSearchDataType> flatMapping, List<String> bucketNames) { MappingResolution combined = MappingResolution.KEYWORD; for (String field : bucketNames) { OpenSearchDataType type = flatMapping.get(field); if (type == null) { return MappingResolution.NOT_AGGREGATABLE; } - if (type.getMappingType() == MappingType.Keyword) { + MappingType mappingType = type.getMappingType(); + if (mappingType == null) { + return MappingResolution.NOT_AGGREGATABLE; + } + if (mappingType == MappingType.Keyword) { continue; } else if (hasKeywordSubField(type)) { combined = MappingResolution.TEXT_WITH_KEYWORD; } else { return MappingResolution.NOT_AGGREGATABLE; } } return combined; } Suggestion importance[1-10]: 7 __ Why: This is a valid defensive programming suggestion. While getMappingType() is unlikely to return null in normal operation, adding a null check prevents potential NullPointerException in edge cases with malformed mappings, improving robustness.	Medium
General	Ensure consistent deep-copy for immutability The single-index branch (size() <= 1) directly puts the original mapping into fieldTypes without deep-copying, while the multi-index branch deep-copies before merging. This inconsistency means lastIndexMappings could still be mutated if a caller modifies fieldTypes when only one index is present. Apply deepCopy() uniformly in both branches to ensure immutability. opensearch/src/main/java/org/opensearch/sql/opensearch/request/system/OpenSearchDescribeIndexRequest.java [110-128] public Map<String, OpenSearchDataType> getFieldTypes() { Map<String, OpenSearchDataType> fieldTypes = new HashMap<>(); Map<String, IndexMapping> indexMappings = client.getIndexMappings(getLocalIndexNames(indexName.getIndexNames())); this.lastIndexMappings = indexMappings; if (indexMappings.size() <= 1) { for (IndexMapping indexMapping : indexMappings.values()) { - fieldTypes.putAll(indexMapping.getFieldMappings()); + fieldTypes.putAll(deepCopy(indexMapping.getFieldMappings())); } } else { - // Merge deep copies: MergeRuleHelper rewrites the accumulated type's nested `properties` in - // place, which would otherwise mutate the per-index mappings retained above (they are reused - // by partial-result partitioning, which needs to see each index's original mapping). for (IndexMapping indexMapping : indexMappings.values()) { MergeRuleHelper.merge(fieldTypes, deepCopy(indexMapping.getFieldMappings())); } } return fieldTypes; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inconsistency in the deep-copy logic. The single-index branch should also use deepCopy() to prevent potential mutation of lastIndexMappings. This improves consistency and prevents subtle bugs, though the impact is moderate since single-index scenarios are less likely to trigger the mutation issue.	Low
	Validate mappings before planning The method retrieves index mappings but doesn't verify they are non-empty before passing to plan(). If getIndexMappings() returns an empty map (e.g., due to a transient cluster state or permission issue), plan() will return null, but the caller won't know whether it was due to no conflict or missing data. Add a defensive check to ensure mappings are present before proceeding. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [470-484] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project, List<String> bucketNames) { if (!QueryContext.isPartialResultEnabled(osIndex.getSettings())) { return null; } - // A format with no warnings channel (CSV/RAW/VIZ) must not silently drop indices. if (!QueryContext.isWarningsSupported()) { return null; } try { Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); + if (mappings == null || mappings.isEmpty()) { + return null; + } PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion to check for empty mappings is reasonable but has limited impact. The plan() method already handles empty mappings by returning null when mappings.size() < 2, so this check is redundant. The getIndexMappings() method is unlikely to return null based on the codebase pattern.	Low

Suggestions up to commit 4bb2d5f

Category	Suggestion	Impact
General	Guard against null index mappings Check that entry.getValue() and entry.getValue().getFieldMappings() are not null before calling traverseAndFlatten. A null mapping could cause a NullPointerException when iterating over entries or flattening the mapping tree. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [68-72] -static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { - if (bucketNames.isEmpty() || mappings.size() < 2) { - return null; +for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { + IndexMapping indexMapping = entry.getValue(); + if (indexMapping == null || indexMapping.getFieldMappings() == null) { + continue; } + Map<String, OpenSearchDataType> flatMapping = + OpenSearchDataType.traverseAndFlatten(indexMapping.getFieldMappings()); - List<String> keywordIndices = new ArrayList<>(); - List<String> textKeywordIndices = new ArrayList<>(); - List<String> excludedIndices = new ArrayList<>(); - for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { - // Flatten so a nested object field (mapping tree resource -> attributes -> applicationid) is - // keyed by its dotted path, matching the bucket field name Calcite resolved. - Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); - Suggestion importance[1-10]: 6 __ Why: Adding null checks for indexMapping and getFieldMappings() is good defensive practice. While the current code assumes valid mappings from getIndexMappings(), the check prevents potential NullPointerException if the contract changes or unexpected data is passed. The suggestion correctly uses continue to skip invalid entries.	Low
	Validate mappings before planning Verify that osIndex.getIndexMappings() is not empty before calling PartialResultAggregatePushdown.plan. An empty mapping could lead to unexpected behavior or null pointer exceptions in downstream logic that assumes at least one index is present. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [479-484] -... +Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); +if (mappings == null || mappings.isEmpty()) { + return null; +} +PartialResultAggregatePushdown.Plan plan = + PartialResultAggregatePushdown.plan(bucketNames, mappings); +if (plan == null) { + return null; +} Suggestion importance[1-10]: 5 __ Why: The suggestion to check for null or empty mappings is reasonable defensive programming. However, getIndexMappings() is documented to return a non-null map (initialized as Map.of() when empty), and the plan method already handles the size() < 2 case. The null check adds safety but is not critical given the existing guard.	Low
	Prevent state leakage on exceptions Ensure clearRequestScopedState is called in all error paths and finally blocks to prevent thread-local state leakage. If an exception occurs before the listener callbacks, the state may persist onto the next pooled thread's request. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [409-413] private static void clearRequestScopedState() { - QueryProfiling.clear(); - QueryContext.setPartialResultOverride(null); - QueryContext.setWarningsSupported(false); + try { + QueryProfiling.clear(); + } finally { + try { + QueryContext.setPartialResultOverride(null); + } finally { + QueryContext.setWarningsSupported(false); + } + } } Suggestion importance[1-10]: 4 __ Why: The nested try-finally blocks ensure each cleanup step runs even if the previous one throws. However, QueryProfiling.clear() and QueryContext setters are unlikely to throw exceptions in practice. The suggestion adds robustness but the improvement is marginal given the existing finally-block placement in wrapWithProfilingClear.	Low

[github-actions]

Persistent review updated to latest commit dad3bb3

[github-actions]

Persistent review updated to latest commit 078c949

[github-actions]

Persistent review updated to latest commit 83fd527

[github-actions]

Persistent review updated to latest commit 2a3eab8

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit a996d24.

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java	128	medium	The `partial_result` field is accepted from the raw client request body and propagated to override the cluster-level setting without any authorization check. Any authenticated PPL user can send `partial_result: true` to force partial-result mode on even when the cluster admin has disabled it via `plugins.query.partial_result.on_mapping_conflict.enabled=false`, bypassing cluster policy.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java	147	low	The warning `detail` field includes concrete excluded index names and field mapping type details (e.g. the exact index name and field path that failed to aggregate). This exposes internal cluster topology — index names and their field schemas — to any user who can issue a wildcard PPL query, which could aid reconnaissance of the OpenSearch cluster structure.
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java	183	low	`QueryContext.setWarningsSupported()` and `QueryContext.setPartialResultOverride()` write into thread-local storage but no explicit cleanup of these keys is shown in this diff. If the underlying thread-pool threads are reused and the thread context is not reset between requests (e.g. on an error path that bypasses normal cleanup), a subsequent request on the same thread could inherit a stale `warnings_supported=true` or a prior request's `partial_result` override, silently enabling partial-result behavior for a request that never requested it.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 19b6187

[github-actions]

Persistent review updated to latest commit 51220fe

[anasalkouz]

1. Is this only applicable for non-mustang?
2. Is this only limited to text vs keyward use-case? can we extend the scope?
3. Shall we have a role on the inspect query feature to suggest customer to enable this parital result flag to optimize performance if the query fails to push down?
4. Can we have performance benchmark for the 3 cases? with no pushdown, with text pushdown, and with partial results?

[ahkcs]

Performance benchmark: partial results vs. today vs. scripted text pushdown (#5646)

Comparing three responses to the mapping-conflict PIT-exhaustion case (an aggregation groups on a field mapped keyword in some indices of a wildcard pattern and text in others):

• A — today. The type merge collapses the field to text-without-doc-values, aggregate pushdown is lost, and the engine scans every document per shard — opening a Point-In-Time (PIT) context on every shard — and aggregates client-side. Complete answer; trips search.max_open_pit_context on wide patterns.
• B — scripted text pushdown (Push down aggregation on text field without .keyword sub-field #5646). Routes the group key through a Calcite _source script pushed down with size=0. No PIT. Complete answer.
• C — partial results (this PR). Narrows the scan to the aggregatable (keyword) subset, pushes down natively (size=0, no PIT), returns a partial answer plus a PARTIAL_RESULT warning naming the excluded indices.

These do not compute the same thing, so every latency figure is paired with a completeness column.

Test setup

Cluster	Single node, OpenSearch 3.8.0-SNAPSHOT, 2 GB heap (raised from the 512 MB dev default so A fails on PIT, not the query memory circuit breaker)
Engine	Calcite path (plugins.calcite.enabled=true) — the only path in scope for this PR
A & C	Same build (this PR); A = partial_result:false, C = partial_result:true
B	#5646 build, separate run on identical seeded data
Iterations	30 measured + 5 warmup per query, serial (clean per-query latency + exact PIT deltas)
Latency	Client-side wall-clock of the _plugins/_ppl call
PIT/query	Delta of the cumulative point_in_time_total node stat

Datasets (deterministic, seed = 42):

• wide — 40 keyword + 4 bare-text indices, 2 shards each (88 shards), 5,000 docs/index (220,000 total). A wide wildcard pattern where 88 shards exceeds any realistic PIT limit.
• small — 1 keyword + 1 text, 1 shard each, 20,000 docs/index. Control below the PIT limit.
• flat — same as small but the conflict field is top-level (appid), not nested. (See the note on B.)

Conflict field for wide/small is a nested resource.attributes.applicationid; for flat it is top-level appid.

Latency p50 / p90 / p99 (ms)

"Today" (A) has two modes on the same query, decided by whether the shard count exceeds search.max_open_pit_context:

Query	A: PIT opened, under limit (no 500)	A: PIT limit exceeded	B: #5646 (script)	C: partial (this PR)	Completeness of C	PIT/query (A)
wide stats	441 / 463 / 494	FAIL — 500	112 / 150 / 271	11 / 12 / 13	91.2% (200k/219k)	88
wide top	439 / 455 / 486	FAIL — 500	123 / 149 / 293	16 / 18 / 21	91.2%	88
small stats	90 / 110 / 115	92 / 111 / 114	32 / 36 / 43	5 / 6 / 7	50% (20k/40k)	2
small top	96 / 116 / 122	94 / 102 / 113	37 / 44 / 53	11 / 14 / 15	50%	2
flat stats	58 / 78 / 82	56 / 63 / 75	20 / 23 / 42	4 / 5 / 5	50% (20k/40k)	4
flat top	60 / 67 / 86	57 / 62 / 80	27 / 30 / 31	10 / 12 / 14	50%	4

The "under limit" column used max_open_pit_context=500; "exceeded" used =10. A only fails where shard count crosses the limit (wide, 88 shards). Small/flat stay under and complete — but still open PITs and run 8–20× slower than C. Error rate in the exceeded regime: A = 100% on wide, C = 0% everywhere (never opens a PIT).

Completeness (sum of count() across all buckets)

Dataset	A (complete)	B	C (partial)	C completeness
wide, nested field	219,328	220,000 but 1 null bucket (grouping lost)	200,000	91.2%
small, nested field	40,000	40,000 but 1 null bucket	20,000	50%
flat field	40,000	40,000, 50 buckets (correct)	20,000	50%

Why the latencies differ (mechanism)

A leaves the aggregate above the scan (explain shows requestedTotalSize=2147483647): every matching document is streamed out of every shard over PIT cursors into the coordinator JVM and counted there — cost scales with document count. B and C fuse the aggregate into the scan (size=0), so the count runs inside each shard and only bucket results cross the wire — cost scales with bucket count, and no PIT is opened. B groups on a per-document _source script; C groups on native keyword doc values, which is why C stays ~2–5× ahead of B even where both push down.

Takeaways

1. When it runs, C is fastest (~34× vs A, ~10× vs B on wide) — but that speed is the partial answer: it excludes the non-aggregatable indices. On wide that is an 8.8% undercount; where the text indices hold half the data, 50%. Always accompanied by the PARTIAL_RESULT warning.
2. In the low-PIT-budget regime, A fails outright (100% errors on wide). B and C never open a PIT.
3. B is complete and PIT-free on flat fields and is the natural default there. On the nested dotted field, B in its current state grouped all documents into a single null bucket (complete count, grouping lost) — worth verifying whether the scripted _source reader resolves nested dotted paths. This PR's producer resolves the nested path.

C is intended as an opt-in escape hatch (default off) for the widest patterns / lowest PIT budgets where a knowingly-partial, clearly-warned answer is preferable to a slow scan or a 500 — complementary to, not competing with, a complete-answer pushdown fix.

_{Single-node, laptop-scale absolutes; the ratios and the PIT / completeness / error-rate columns are the transferable results.}

[ahkcs]

1. Is this only applicable for non-mustang?
2. Is this only limited to text vs keyward use-case? can we extend the scope?
3. Shall we have a role on the inspect query feature to suggest customer to enable this parital result flag to optimize performance if the query fails to push down?
4. Can we have performance benchmark for the 3 cases? with no pushdown, with text pushdown, and with partial results?

1. Yes, currently it's only applicable for Calcite path.
2. Today it's deliberately scoped to the text/keyword conflict, it can be extended, and the shape generalizes cleanly if we have more partial result use cases.
3. We can add that recommendation/suggestion
4. link for performance benchmarking: Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts #5657 (comment)

[github-actions]

Persistent review updated to latest commit 7db73c0

[github-actions]

Persistent review updated to latest commit ad3c0a5

[github-actions]

Persistent review updated to latest commit cbf5074

[github-actions]

Persistent review updated to latest commit 11b57b2