Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts by ahkcs · Pull Request #5657 · opensearch-project/sql

ahkcs · 2026-07-27T21:48:18Z

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

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."
}]

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.
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 · 2026-07-27T21:50:18Z

PR Reviewer Guide 🔍

(Review updated until commit `77fa72d`)

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 `stringOf` helper catches all `RuntimeException` to handle non-string bucket keys, but this is overly broad. If `content.stringValue()` throws an unexpected runtime exception (e.g., `NullPointerException` from a bug elsewhere), the catch block silently converts it to a string representation instead of propagating the failure. This masks genuine errors that should surface during development or in production logs. A narrower catch (e.g., `ClassCastException` or a custom exception from the Content API) would be safer. private static String stringOf(Content content) { try { return content.stringValue(); } catch (RuntimeException e) { // Not a string value (e.g. a numeric aggregation bucket key landing in a text column via a // partial-result narrowing) -- render its string form instead of failing the cast to null. return String.valueOf(content.objectValue()); } } Possible Issue `resolvePartitionFields` returns `null` when a group key is a pure constant (no input refs), signaling that partitioning cannot proceed. However, the caller `tryPartialResultAggregate` does not distinguish this case from other failures (e.g., exceptions). If a query legitimately groups by a constant and the partial-result path is enabled, the `null` return causes the method to silently skip partial mode without logging why. This could confuse operators debugging why a query did not use partial mode when expected. Consider logging a debug message when returning `null` for a constant key, or document this behavior clearly. / @Nullable private List<String> resolvePartitionFields(Aggregate aggregate, @Nullable Project project) { List<String> scanFields = getRowType().getFieldNames(); List<String> fields = new ArrayList<>(); for (int group : aggregate.getGroupSet()) { Set<Integer> refs = new LinkedHashSet<>(); if (project == null) { refs.add(group); // group key indexes directly into the scan } else { project .getProjects() .get(group) .accept( new RexVisitorImpl<Void>(true) { @Override public Void visitInputRef(RexInputRef ref) { refs.add(ref.getIndex()); return null; } }); } if (refs.isEmpty()) { return null; // constant group key -> nothing to partition on } for (int ref : refs) { String name = scanFields.get(ref); if (!fields.contains(name)) { fields.add(name); } } } return fields; } Possible Issue* The `resolveBucketSignature` method returns `null` for any field that is absent from an index's mapping. This means an index missing the group field is always excluded, even if the field is aggregatable in other indices. While this is correct for the current use case (a missing field cannot be aggregated), the method does not distinguish between "field is text" and "field is absent". If a future caller needs to know why an index was excluded (e.g., to emit a more specific warning), this conflation could cause confusion. Consider returning a distinct sentinel or documenting this behavior explicitly. @Nullable static String resolveBucketSignature( Map<String, OpenSearchDataType> flatMapping, List<String> bucketNames) { List<String> tokens = new ArrayList<>(); for (String field : bucketNames) { OpenSearchDataType type = flatMapping.get(field); if (type == null) { return null; // field absent here -> not aggregatable } MappingType mappingType = type.getMappingType(); if (mappingType == MappingType.Keyword) { tokens.add("kw"); } else if (mappingType == MappingType.Text \|\| mappingType == MappingType.MatchOnlyText) { return null; // text family (incl. text-with-.keyword) collapses to bare text on merge } else { tokens.add("t:" + mappingType); // other aggregatable type (numeric, date, boolean, ip) } } return String.join("\|", tokens); }

github-actions · 2026-07-27T21:50:52Z

PR Code Suggestions ✨

Latest suggestions up to 77fa72d

Explore these optional code suggestions:

Category Suggestion Impact

Possible issue

Validate non-empty kept indices list

The narrowedIndex is created with a comma-joined list of index names without
validation. If plan.keptIndices() is empty, this will create an index with an empty
string name, which could cause unexpected behavior. Verify that keptIndices() is
non-empty before creating the narrowed index.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [520-530]

-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();
-    PartialResultAggregatePushdown.Plan plan =
-        PartialResultAggregatePushdown.plan(partitionFields, mappings);
-    if (plan == null) {
-      return null;
-    }
-
-    OpenSearchIndex narrowedIndex =
-        new OpenSearchIndex(
-            osIndex.getClient(), osIndex.getSettings(), String.join(",", plan.keptIndices()));
-    ...
-  }
+if (plan == null) {
+  return null;
+}
+if (plan.keptIndices().isEmpty()) {
+  return null;
 }
 
+OpenSearchIndex narrowedIndex =
+    new OpenSearchIndex(
+        osIndex.getClient(), osIndex.getSettings(), String.join(",", plan.keptIndices()));
+

Suggestion importance[1-10]: 7

__

Why: Valid concern about potential empty keptIndices() list leading to an invalid index name. However, the suggestion asks to verify/ensure a condition rather than fixing a definite bug, so it should not score above 7.

Medium

Add null check for mapping type

The method assumes type.getMappingType() never returns null, but if it does, the
subsequent checks will fail with a NullPointerException. Add a null check for
mappingType before using it in comparisons to prevent potential crashes.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [110-125]

-static String resolveBucketSignature(
-    Map<String, OpenSearchDataType> flatMapping, List<String> bucketNames) {
-  List<String> tokens = new ArrayList<>();
-  for (String field : bucketNames) {
-    OpenSearchDataType type = flatMapping.get(field);
-    if (type == null) {
-      return null; // field absent here -> not aggregatable
-    }
-    MappingType mappingType = type.getMappingType();
-    if (mappingType == MappingType.Keyword) {
-      tokens.add("kw");
-    } else if (mappingType == MappingType.Text || mappingType == MappingType.MatchOnlyText) {
-      return null; // text family (incl. text-with-.keyword) collapses to bare text on merge
-    } else {
-      tokens.add("t:" + mappingType); // other aggregatable type (numeric, date, boolean, ip)
-    }
-  }
-  return String.join("|", tokens);
+MappingType mappingType = type.getMappingType();
+if (mappingType == null) {
+  return null; // mapping type unavailable -> not aggregatable
+}
+if (mappingType == MappingType.Keyword) {
+  tokens.add("kw");
+} else if (mappingType == MappingType.Text || mappingType == MappingType.MatchOnlyText) {
+  return null;
+} else {
+  tokens.add("t:" + mappingType);
 }

Suggestion importance[1-10]: 7

__

Why: The suggestion addresses a potential NullPointerException if getMappingType() returns null. This is a reasonable defensive check, though it's unclear if this scenario can actually occur in practice. As error handling, it should not score above 8, and since it's asking to verify a condition, it caps at 7.

Medium

Previous suggestions

Suggestions up to commit 0db5aa7

Category	Suggestion	Impact
General	Pre-flatten mappings to avoid redundant traversals The `traverseAndFlatten` call inside the loop is invoked once per index, which can be expensive for large wildcard patterns. Consider pre-flattening all mappings once before the loop, or caching the flattened result in `IndexMapping` itself to avoid redundant traversals. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [71-76] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { if (bucketNames.isEmpty() \|\| mappings.size() < 2) { return null; } + // Pre-flatten all mappings once + Map<String, Map<String, OpenSearchDataType>> flatMappings = new LinkedHashMap<>(); + for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { + flatMappings.put(entry.getKey(), + OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings())); + } ... - for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { - Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); - switch (resolveBucketMapping(flatMapping, bucketNames)) { + for (Map.Entry<String, Map<String, OpenSearchDataType>> entry : flatMappings.entrySet()) { + switch (resolveBucketMapping(entry.getValue(), bucketNames)) { Suggestion importance[1-10]: 5 __ Why: Valid optimization to avoid repeated `traverseAndFlatten` calls in the loop. However, the impact is moderate since this only runs when partial mode applies (opt-in setting, multi-index conflict), and the flattening cost is typically small compared to the query execution itself.	Low
	Guard against stale thread-local state outside request lifecycle The method reads from `ThreadContext` without verifying that the context is properly initialized for the current request. If called outside a request lifecycle (e.g., during initialization or in a background thread), it may return stale data from a previous request on the same pooled thread. Add a guard or document the lifecycle requirement. 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); + // Only check override if a request ID is set (indicating active request context) + if (ThreadContext.get(REQUEST_ID_KEY) != null) { + 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); } Suggestion importance[1-10]: 4 __ Why: The concern about stale state is valid, but the PR already addresses cleanup in `TransportPPLQueryAction.clearRequestScopedState()` (lines 406-408 in `TransportPPLQueryAction.java`), which clears the override after each request. The suggested guard adds defensive depth but may be unnecessary given the existing cleanup.	Low
	Defer partial-result attempt until after normal pushdown fails The partial-result path is attempted before the normal aggregation analysis, which means it may fire even when the aggregation would have succeeded normally. Consider moving this check after the initial `AggregateAnalyzer` attempt, so partial mode only activates when a conflict actually prevents pushdown. This avoids unnecessary partitioning overhead for queries that don't need it. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [420-427] -List<String> partitionFields = resolvePartitionFields(aggregate, project); -if (partitionFields != null) { - AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields); - if (partial != null) { - return partial; +try { + // ... existing analyze logic ... + AggregateAnalyzer.AggregateBuilderHelper helper = ...; + // If analysis succeeds, return the pushed aggregate +} catch (Exception e) { + // Only try partial mode if normal pushdown failed + List<String> partitionFields = resolvePartitionFields(aggregate, project); + if (partitionFields != null && allowPartialFallback) { + AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields); + if (partial != null) { + return partial; + } } + throw e; } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the PR's design. The comment at line 418-419 explicitly states partial mode must be tried before analyze because since #5646 a text/keyword conflict pushes down as a slow script instead of failing, so a post-failure fallback would never fire. Moving the check after analysis would break the feature.	Low

Suggestions up to commit cafac5f

Category	Suggestion	Impact
Possible issue	Clear all thread-local state consistently The `clearRequestScopedState` method clears thread-local state but does not clear the `CalcitePlanContext` warnings that are also thread-local. This could cause warnings from one request to leak into the next request on a pooled thread. Ensure all thread-local state is cleared consistently. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [404-408] private static void clearRequestScopedState() { QueryProfiling.clear(); QueryContext.setPartialResultOverride(null); QueryContext.setWarningsSupported(false); + CalcitePlanContext.drainWarnings(); } Suggestion importance[1-10]: 8 __ Why: Valid concern: `CalcitePlanContext.drainWarnings()` is called in `OpenSearchExecutionEngine.buildResultSet` (line 495), but if an exception occurs before that point, warnings could leak onto the next pooled thread. Adding `drainWarnings()` to `clearRequestScopedState` ensures warnings are always cleared, preventing potential cross-request contamination.	Medium
General	Cache flattened mappings to avoid redundant operations The `traverseAndFlatten` operation is called for every index mapping in the loop, which could be expensive for large wildcard patterns with many indices. Consider caching the flattened mappings or performing this operation once per unique mapping structure to improve performance. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [68-74] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { if (bucketNames.isEmpty() \|\| mappings.size() < 2) { return null; } - ... + + Map<String, Map<String, OpenSearchDataType>> flattenedMappings = new LinkedHashMap<>(); for (Map.Entry<String, IndexMapping> entry : mappings.entrySet()) { - Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); - String signature = resolveBucketSignature(flatMapping, bucketNames); + flattenedMappings.put( + entry.getKey(), + OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings())); + } + + Map<String, List<String>> aggregatableGroups = new LinkedHashMap<>(); + List<String> nonAggregatable = new ArrayList<>(); + for (Map.Entry<String, Map<String, OpenSearchDataType>> entry : flattenedMappings.entrySet()) { + String signature = resolveBucketSignature(entry.getValue(), bucketNames); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that `traverseAndFlatten` is called in a loop, but the optimization is minor: the method is already efficient for typical wildcard patterns, and the added complexity of caching may not justify the marginal performance gain. The existing code is clear and correct.	Low
General	Move partial-result attempt after normal pushdown The partial-result fallback is attempted before the main aggregation analysis, which means it may fire even when the aggregation would have succeeded normally. Consider moving the partial-result attempt into the catch block or after detecting a specific failure condition to avoid unnecessary overhead when the aggregation can be pushed down cleanly. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [390-428] private AbstractRelNode pushDownAggregate( Aggregate aggregate, @Nullable Project project, boolean allowPartialFallback) { try { CalciteLogicalIndexScan newScan = new CalciteLogicalIndexScan( getCluster(), ... + // Attempt normal pushdown first + AggregateAnalyzer.AggregateBuilderHelper helper = ... + // Only try partial fallback if normal pushdown fails + } catch (Exception e) { if (allowPartialFallback) { List<String> partitionFields = resolvePartitionFields(aggregate, project); if (partitionFields != null) { AbstractRelNode partial = tryPartialResultAggregate(aggregate, project, partitionFields); if (partial != null) { return partial; } } } + if (LOG.isDebugEnabled()) { + LOG.debug("Cannot pushdown the aggregate {}", aggregate, e); + } + } + return null; +} Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the design: the comment at line 418 explicitly states partial mode must be tried before analyze because since #5646 a text/keyword conflict pushes down as a slow script instead of failing. Moving it to the catch block would defeat the purpose, as the fallback would never fire. The current placement is intentional.	Low

Suggestions up to commit 11b57b2

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
Possible issue	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

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
Possible issue	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
General	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

github-actions · 2026-07-27T21:55:01Z

Persistent review updated to latest commit dad3bb3

github-actions · 2026-07-27T22:00:35Z

Persistent review updated to latest commit 078c949

github-actions · 2026-07-28T18:23:21Z

Persistent review updated to latest commit 83fd527

github-actions · 2026-07-28T20:22:40Z

Persistent review updated to latest commit 2a3eab8

github-actions · 2026-07-28T20:40:47Z

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 · 2026-07-28T20:41:56Z

Persistent review updated to latest commit 19b6187

github-actions · 2026-07-28T20:55:08Z

Persistent review updated to latest commit 51220fe

anasalkouz · 2026-07-28T21:38:24Z

Is this only applicable for non-mustang?
Is this only limited to text vs keyward use-case? can we extend the scope?
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?
Can we have performance benchmark for the 3 cases? with no pushdown, with text pushdown, and with partial results?

ahkcs · 2026-07-29T00:05:12Z

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

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.
In the low-PIT-budget regime, A fails outright (100% errors on wide). B and C never open a PIT.
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 · 2026-07-29T16:49:02Z

Is this only applicable for non-mustang?

Is this only limited to text vs keyward use-case? can we extend the scope?

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?

Can we have performance benchmark for the 3 cases? with no pushdown, with text pushdown, and with partial results?

Yes, currently it's only applicable for Calcite path.
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.
We can add that recommendation/suggestion
link for performance benchmarking: Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts #5657 (comment)

Trim the warning detail to the essentials for an end user: which field was not keyword everywhere, which indices were excluded, and the single remedy (map the field as keyword across all indices). Drops the doc-values / wildcard-merge mechanics, and removes the earlier suggestion that a text field with a keyword sub-field is an acceptable mapping -- under a wildcard it still merges to text and is not aggregatable, so keyword is the only reliable fix to recommend. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The warnings-supported check called format() on every request, including explain requests whose format is an explain-only value (json/yaml) that Format.of() does not recognize -- so an _explain request failed with 'response in json format is not supported' before reaching the explain branch. Skip the check for explain requests, which never carry query warnings anyway. Fixes the doctest failures on docs/user/ppl/interfaces/endpoint.md. Signed-off-by: Kai Huang <ahkcs@amazon.com>

…erage The protocol module requires 100% branch coverage. QueryResult's warnings constructor normalizes null to an empty list, but no test exercised the null branch, dropping protocol branch coverage to 0.9 and failing jacocoTestCoverageVerification. Add a QueryResultTest case covering the no-warnings, provided-list, and null-list paths. Signed-off-by: Kai Huang <ahkcs@amazon.com>

…ck into pushDownAggregate The setting is user-facing behavior, not a Calcite internal, so move it from plugins.calcite.* to plugins.query.partial_result.on_mapping_conflict.enabled and drop the CALCITE_ prefix from the key. Fold tryPartialResultAggregate into pushDownAggregate so the planner rule keeps a single entry point. The fallback is now private and gated by an allowPartialFallback flag, so re-entering on the narrowed scan attempts it at most once. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The partial-result path needs per-index mappings to decide which indices are aggregatable, but the merged field types cached on OpenSearchIndex discard that detail, so it was re-requesting the mappings from the client. Retain the per-index mappings on the describe request that already fetches them and cache them alongside the merged types, so partitioning reuses that result instead of issuing a second mapping request. Also collapses three copies of the fetch-and-cache block into one helper. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The shorter plugins.query.* key fits on one line, so the wrapped form no longer matches google-java-format. Signed-off-by: Kai Huang <ahkcs@amazon.com>

…ion bug The optimization had partial-result partitioning reuse the per-index mappings cached on OpenSearchIndex. But getFieldTypes() merges those mappings with MergeRuleHelper, and DeepMergeRule.mergeInto mutates the target's nested 'properties' map in place -- and that target aliases the first-iterated index's OpenSearchDataType objects. Reusing the cached mappings therefore handed the partitioner a mapping whose nested field had been merged into the sibling index's type, so a text/keyword conflict on a nested field intermittently classified as no-conflict, returned no partitioning plan, and fell through to the PIT-exhausting scan. The outcome depended on map iteration order, hence the flaky CalcitePartialResultOnMappingConflictIT.partialResultOnHandlesNestedDottedField. Restore the direct getIndexMappings() fetch, which returns freshly-parsed mappings immune to that mutation. This only runs on the opt-in partial path after normal pushdown has already failed (a cold path), so the extra fetch is acceptable. The underlying in-place-merge mutation is a separate latent issue. Stress-verified: reverted code passes the full IT class 8/8; the optimized code failed 4/5. Signed-off-by: Kai Huang <ahkcs@amazon.com>

… them Partial-result partitioning needs per-index mappings, which the merged field types cached on OpenSearchIndex discard, so it was fetching them a second time. Retain the per-index mappings on the describe request that already fetches them and cache them alongside the merged types, so partitioning reuses that result. The first attempt at this was reverted because MergeRuleHelper rewrites the accumulated type's nested properties in place, mutating the very mappings being retained: a nested text/keyword conflict then read back as no conflict, produced no partitioning plan, and fell through to the PIT-exhausting scan. Merge deep copies instead, via a new OpenSearchDataType.cloneDeep() that carries the nested properties subtree (cloneEmpty drops it). Covered by a regression test that fails without the copy. Stress-verified: CalcitePartialResultOnMappingConflictIT passes 8/8 (it failed 4/5 before). Signed-off-by: Kai Huang <ahkcs@amazon.com>

The partial-result override and the warnings-supported flag live in QueryContext's log4j thread-locals, but only QueryProfiling was being cleared when a request finished. Transport threads are pooled, so a query that expressed no preference inherited the previous query's override from the same thread: with the cluster setting off and no request flag, an aggregation over a text/keyword conflict intermittently returned a partial result (with a warning) instead of failing -- observed 7 of 12 runs after an earlier request had set the flag. Clear both flags alongside QueryProfiling in the response listener. Verified: flag-absent requests now fail 12/12 when interleaved with explicit true requests, while explicit true still returns the partial result. Signed-off-by: Kai Huang <ahkcs@amazon.com>

…VersionUID OpenSearchDataType is Serializable without an explicit serialVersionUID, so the JVM derives one from the class shape. Adding cloneDeep() changed it, and that UID is embedded in the Java-serialized script blobs these two explain plans assert on. Both files now carry the same derived UID (7128bdc1452f35d3). The ppl/ one is confirmed by ExplainIT passing; the calcite/ one is skipped in this environment (enabledOnlyWhenPushdownIsEnabled) and verified by decoding both blobs and comparing the UID bytes. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The partial-result path hooked the failure branch of pushDownAggregate: a group key that collapsed to text-without-keyword used to throw (getReferenceForTermQuery returned null and the composite builder rejected it), and the fallback caught that. opensearch-project#5646 made that case succeed instead -- it pushes down as a per-document _source script -- so the fallback lost its trigger and the setting became a no-op. Verified by cherry-picking opensearch-project#5646 onto this branch: 7 of 10 ITs failed, the partial-result ones because pushdown now succeeds and no warning is emitted. Consult the partial-result plan before AggregateAnalyzer.analyze instead. The choice is no longer failure-vs-fallback but between two working plans: a native aggregation over the keyword subset (fast, incomplete, warned) and opensearch-project#5646's script over every document (slow, complete). Only an up-front check can pick the fast one. The post-failure call is kept so a key that genuinely cannot push down (e.g. an array bucket) still gets the chance. Two ITs asserted the old failure mode (PIT exhaustion raising a 4xx). That failure no longer happens, which is the point of opensearch-project#5646, so they now assert the behavior that matters: partial-result off returns the complete result with no warning, and CSV -- which has no warnings channel -- still returns every index rather than silently dropping one. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Add a settings.rst entry for plugins.query.partial_result.on_mapping_conflict.enabled: what a text/keyword mapping conflict is, the complete-but-slow default vs the fast-but-partial opt-in, the PARTIAL_RESULT warning, the JSON-only constraint, and the per-request partial_result override. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Signed-off-by: Kai Huang <ahkcs@amazon.com>

- settings.rst: mark the setting [Experimental] with a note, and correct the version to 3.9. - Consolidate the per-request-override + cluster-setting precedence into QueryContext.isPartialResultEnabled(Settings); drop the duplicate resolver in CalciteLogicalIndexScan and the getPartialResultOverride accessor. - Remove a redundant inline comment. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The partial-result check runs before analyze (line ~418); the two post-failure call sites could never add a case. The catch-path call re-invoked with identical inputs the pre-analyze check already tried, so it always returned null. The array/nested branch is issue opensearch-project#5006's scope, not a text/keyword conflict, so partial mode does not apply. Both revert to returning null, and the now-unused two-arg overload is removed. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-17T18:46:17Z

Persistent review updated to latest commit 7db73c0

A group field mapped keyword in some indices and a non-text type (e.g. int) in others is a type conflict, not a text/keyword collapse. The int index is aggregatable, so excluding it would silently drop valid data and mislabel it a text/keyword conflict. Classify such a field as CONFLICTING_TYPE and return no plan, leaving the query to the normal path (the type conflict itself is out of scope here). Bare text and absent fields are still excludable as before. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-17T18:55:32Z

Persistent review updated to latest commit ad3c0a5

Resolve each aggregation group key through the eval Project to the scan fields it reads, so an expression key (e.g. eval g = lower(city) | stats count() by g) gets partial results over the keyword subset just like a bare 'by city'. Previously only a bare group field matched the per-index mapping; a derived key looked up its output alias, found nothing, and bailed to the complete (script) path. A constant group key resolves to no field and cleanly bails. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-17T21:51:33Z

Persistent review updated to latest commit cbf5074

Covers concat(city, region) over a text/keyword conflict: the key traces to both fields, keeps only the index where both are aggregatable, and warns naming both fields and the excluded index. Closes the end-to-end gap on multi-field expression keys. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-17T22:14:06Z

Persistent review updated to latest commit 11b57b2

Generalize partitioning from a text/keyword-only enum to a per-index compatibility signature. This also covers a single aggregatable non-text type mixed with bare text (e.g. integer vs text): keep the aggregatable index, exclude the text one, and warn -- rather than silently coercing to one type and dropping the other index's docs. A conflict between mutually-incompatible aggregatable types (keyword vs integer, two numeric types) is left to the normal path: its merged type is an arbitrary last-write-wins, so narrowing to any one subset could misread the other's values under that type. That is a fundamental type conflict tracked separately (opensearch-project#5610). Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-20T21:33:26Z

Persistent review updated to latest commit cafac5f

Live testing showed the non-text generalization is unsafe. The narrowed scan reuses the conflict's merged output type, which for a non-text conflict is an arbitrary last-write-wins. When int-vs-text merged to text, keeping the int index produced a native numeric aggregation whose integer bucket keys did not materialize under the text output column -- the group labels came back null ([[2, null], [1, null]]). And when the merge instead picks text, the normal path already returns the complete result, so narrowing only loses data. Only the text/keyword collapse narrows safely: its merged type is a deterministic text, and a kept keyword / text-with-.keyword group's string bucket keys match it. Reverting to that scope. keyword-vs-int and other mutually-incompatible aggregatable-type conflicts remain on the normal path (a fundamental type conflict, opensearch-project#5610). Expression-key tracing (#cbf50748) is unaffected and retained. This reverts commit cafac5f. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-20T21:41:16Z

Persistent review updated to latest commit 0db5aa7

…nflict Partition by aggregatability, not just text/keyword: an index whose group field is non-aggregatable is dropped and the aggregatable indices are kept. Non-aggregatable = the text family (text, text-with-.keyword, match_only_text -- all collapse to bare text on merge) plus absent fields. Aggregatable = keyword, numerics, date, boolean, ip. So e.g. integer-vs-text now keeps the integer index and excludes the text one, warning about the exclusion, instead of silently coercing to one type and dropping the other index's docs. Kept indices must share one aggregatable type; a mix of incompatible aggregatable types (keyword vs integer, two numeric types) has an arbitrary last-write-wins merged type and is left to the normal path (opensearch-project#5610). Also coerce a numeric/boolean aggregation bucket key to its string form when it lands in a text-typed output column (OpenSearchExprValueFactory), rather than failing the cast and nulling the label -- which happens when the kept non-keyword index's native buckets flow through the conflict's text-merged output type. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-08-20T22:11:58Z

Persistent review updated to latest commit 77fa72d

ahkcs added the enhancement New feature or request label Jul 27, 2026

ahkcs marked this pull request as ready for review July 28, 2026 21:26

ahkcs requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners July 28, 2026 21:26

ahkcs added 16 commits August 17, 2026 11:39

Fix formatting of the renamed partial-result setting key

7f953f9

The shorter plugins.query.* key fits on one line, so the wrapped form no longer matches google-java-format. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Tighten inline comments on the partial-result path

b6f543a

Signed-off-by: Kai Huang <ahkcs@amazon.com>

Tighten inline comments

7db73c0

Signed-off-by: Kai Huang <ahkcs@amazon.com>

ahkcs force-pushed the feature/ppl-partial-result-warning-channel branch from c06668d to 7db73c0 Compare August 17, 2026 18:44

Uh oh!

Conversation

ahkcs commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How it works

Behavior

Key points

Related Issues

Check List

Uh oh!

github-actions Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 77fa72d)

Uh oh!

github-actions Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

anasalkouz commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahkcs commented Jul 29, 2026

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

Test setup

Latency p50 / p90 / p99 (ms)

Completeness (sum of count() across all buckets)

Why the latencies differ (mechanism)

Takeaways

Uh oh!

ahkcs commented Jul 29, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahkcs commented Jul 27, 2026 •

edited

Loading

github-actions Bot commented Jul 27, 2026 •

edited

Loading

(Review updated until commit `77fa72d`)

github-actions Bot commented Jul 27, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

anasalkouz commented Jul 28, 2026 •

edited

Loading

Completeness (sum of `count()` across all buckets)