Return a partial result with a warning instead of exhausting PIT contexts on a text/keyword mapping conflict by ahkcs · Pull Request #5657 · opensearch-project/sql

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

Description

On the Calcite PPL path, an aggregation (stats/top/rare) that groups, sorts, or dedups on a field whose mapping conflicts across a wildcard index pattern — keyword in some indices, text in others — cannot push down. The type merge must pick one type valid on every shard, so it collapses the field to text-without-.keyword; text has no doc values, so the aggregation falls back to an in-process document scan that opens a Point-In-Time (PIT) context on every shard of every matched index. On a wide pattern this trips search.max_open_pit_context and the customer sees an opaque failure.

This PR adds an opt-in fallback that returns a partial result plus a structured warning instead of failing. When normal aggregate pushdown fails on such a conflict and partial mode is enabled, the engine narrows the scan to the subset of indices where the field is aggregatable (mapped as keyword), pushes the aggregation down over just that subset (so size = 0 and no PIT is opened), and attaches a PARTIAL_RESULT warning naming the excluded indices.

Three parts:

A general warning channel. Successful PPL responses gain an optional warnings array ({type, message, detail}), emitted only when non-empty so existing responses are byte-for-byte unchanged:

{
  "schema": [ ... ], "datarows": [ ... ], "total": 3, "size": 3,
  "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."
    }
  ]
}

The detail truncates the excluded-index list (a few names + "and N more") so a wide pattern stays readable.

The partial-result producer (PartialResultAggregatePushdown), wired into AggregateIndexScanRule as the fallback when normal aggregate pushdown returns null. Partitioning/selection logic is unit-tested in isolation.
A per-request override. An optional partial_result request-body flag (mirroring profile) lets a client — e.g. an OpenSearch Dashboards toggle — turn partial mode on/off per query. When present it overrides the cluster setting (true forces on, false forces off); when absent the cluster setting decides.

Behavior (partial mode off vs on), on a keyword+text wildcard pattern with search.max_open_pit_context below the shard count:

Query	Off (today)	On
`stats count() by <conflict field>`	500 — PIT contexts exhausted	200 — counts over the aggregatable subset + `PARTIAL_RESULT` warning, 0 PITs
`top` / `rare <conflict field>`	500 — PIT contexts exhausted	200 — partial buckets + warning, 0 PITs
`stats count() by span(...)` (no conflict)	200 — complete	200 — complete, no warning (unchanged)
single-index / no-conflict aggregation	200 — complete	200 — complete, no warning (unchanged)
any of the above with `format=csv`	500 / 200 as above	falls through to the normal path (CSV has no warning channel)

Key design points

Opt-in, default off. Gated by the cluster setting plugins.calcite.partial_result.on_mapping_conflict.enabled (default false) and/or the per-request partial_result flag. A partial result is a knowingly-incomplete answer, so it never happens silently, and with nothing enabled the change is byte-for-byte behavior-preserving.
Refuse if the format can't warn. Only the JSON response shape carries the warnings array, so partial mode is skipped for CSV/RAW/VIZ — those fall through to the normal (failing) path rather than returning a silently-partial answer.
Which indices are kept is deterministic, not count-based. Always keep the keyword group when any keyword index exists (the canonical aggregatable representation); fall back to the text-with-.keyword group only when there is no keyword index; always exclude bare-text. The returned data does not depend on how many indices of each type happen to match.
Scoped to the Calcite PPL path; the V2/legacy path is untouched.

Not in scope: recovering an excluded but aggregatable group (e.g. text-with-.keyword when a keyword group is also present) — that needs a per-group split-and-union, a larger separate change. Partial mode deliberately trades that completeness for a single narrowed scan and a clear warning. (This is also why the warning recommends mapping the field as keyword everywhere: under a wildcard, a .keyword sub-field still merges to plain text and is not aggregatable.)

Related Issues

Check List

New functionality includes testing (unit + integration).
New functionality has been documented (warning is self-describing; setting registered).
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.

Introduce a first-class warnings array on the query response so the engine can attach non-fatal, structured notices (type/message/detail) to an otherwise-successful result, without turning it into an error. This is the prerequisite for returning a labeled partial result instead of failing a query that would otherwise exhaust Point-In-Time contexts. - Add Warning value type and QueryResponse.warnings (core). - Collect warnings during Calcite planning via a thread-local on CalcitePlanContext, drained in OpenSearchExecutionEngine.buildResultSet and cleared with the other lifecycle signals so nothing leaks across queries. - Thread warnings through QueryResult and emit them in SimpleJsonResponseFormatter only when non-empty, so existing responses are byte-for-byte unchanged. Scoped to the Calcite PPL path. Signed-off-by: Kai Huang <ahkcs@amazon.com>

When an aggregation groups by a field that a text/keyword mapping conflict collapsed to text-without-keyword across a wildcard pattern, pushdown fails and the query falls back to a per-shard document scan that opens a Point-In-Time context on every shard, tripping search.max_open_pit_context. Add an opt-in fallback: when normal aggregate pushdown fails and plugins.calcite.partial_result.on_mapping_conflict.enabled is set, narrow the scan to the largest homogeneous subset of indices whose mapping of the grouped field is aggregatable, push the aggregation down over just that subset (size=0, no PIT), and attach a PARTIAL_RESULT warning naming the excluded indices. - New default-off setting, registered in OpenSearchSettings. - tryPartialResultAggregate in CalciteLogicalIndexScan partitions the matched indices, keeps the keyword group (or text-with-keyword if no keyword group), rebinds the pushdown context to the narrowed index preserving pushed operations such as a WHERE filter, and re-runs pushdown. - Wired into AggregateIndexScanRule as the fallback when pushdown returns null. - drainWarnings de-duplicates, since the planner may raise the warning for multiple equivalent plan alternatives. - Integration test verifies: partial off fails with PIT exhaustion; partial on returns the keyword-index counts with the warning and no PIT; a conflict-free aggregation attaches no warning. Scoped to the Calcite PPL path. Signed-off-by: Kai Huang <ahkcs@amazon.com>

A partial result is only safe if the response can carry the warning that says so. Refuse partial mode (fall through to the normal path) when the requested format has no warnings channel -- CSV, RAW, and VIZ -- so a knowingly-partial result is never returned silently. - QueryContext carries a warnings-supported flag, set in TransportPPLQueryAction from the request format (true only for the JSON shape), read by the producer. - tryPartialResultAggregate bails when warnings are unsupported. - Integration test: partial on + format=csv still errors on PIT exhaustion rather than returning a silently-partial CSV. Signed-off-by: Kai Huang <ahkcs@amazon.com>

The partial-result partitioner looked up the grouped field in each index's raw field mappings by its dotted name. A nested/object field such as resource.attributes.applicationid is stored as an object tree, not a flat dotted key, so the lookup returned null, every index classified as NOT_AGGREGATABLE, and the producer bailed -- leaving the query to exhaust PIT contexts. This is the exact shape of the real observability field that motivated the feature. Flatten each index's field mappings with OpenSearchDataType.traverseAndFlatten (the same flattening the field-type resolver uses) before the lookup, so the dotted bucket name resolves. Add an integration test over a nested-field conflict pattern. Found by live testing the customer query; the flat-field integration test missed it. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Two refinements to the partial-result partitioner: - Pick the kept index group by a deterministic priority instead of a count-based majority: always keep the keyword group when any keyword index exists (the canonical aggregatable representation), fall back to the text-with-.keyword group only when there is no keyword index, and always exclude bare-text. The returned data no longer depends on how many indices of each type match, so a stray index can't flip which subset the user sees. - Correct the warning wording: the old remedy ("add a .keyword sub-field") was misleading when the excluded index already had one. Reword to say the aggregation ran over the largest consistently-mapped subset and to suggest aligning the mapping (e.g. keyword everywhere). Add an integration test where keyword is outnumbered 2:1 by text-with-.keyword indices and must still be the kept group. Signed-off-by: Kai Huang <ahkcs@amazon.com>

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

PR Reviewer Guide 🔍

(Review updated until commit `cdfab96`)

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 `lastIndexMappings` field is reassigned on every call to `getFieldTypes()`, but the method can be called multiple times (e.g., by `getFieldTypes()` and `getAliasMapping()` in sequence). If the underlying `client.getIndexMappings()` returns different results on successive calls (e.g., due to index changes or caching behavior), the retained mappings may not match the merged field types. This could cause partial-result partitioning to see stale or inconsistent mappings. public Map<String, OpenSearchDataType> getFieldTypes() { Map<String, OpenSearchDataType> fieldTypes = new HashMap<>(); Map<String, IndexMapping> indexMappings = client.getIndexMappings(getLocalIndexNames(indexName.getIndexNames())); this.lastIndexMappings = indexMappings; Possible Issue The `tryPartialResultAggregate` method constructs a narrowed `OpenSearchIndex` with a comma-joined index list, but does not validate that `plan.keptIndices()` is non-empty. If the plan returns an empty kept-indices list (which `PartialResultAggregatePushdown.plan` should prevent but is not enforced here), `String.join(",", emptyList)` produces an empty string, leading to an invalid index name and likely a downstream error. CalciteLogicalIndexScan narrowedScan = new CalciteLogicalIndexScan( getCluster(), Possible Issue The `clearRequestScopedState()` method clears `QueryContext.setPartialResultOverride(null)` and `QueryContext.setWarningsSupported(false)`, but does not clear the request ID or profile flag set by `QueryContext.addRequestId()` and `QueryContext.setProfile()`. If these are also per-request state on pooled threads, they may leak onto the next query. If they are intentionally not cleared, the comment should clarify why. * request's override. */ private static void clearRequestScopedState() { QueryProfiling.clear(); QueryContext.setPartialResultOverride(null); QueryContext.setWarningsSupported(false); }

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

PR Code Suggestions ✨

Latest suggestions up to cdfab96

Explore these optional code suggestions:

Category Suggestion Impact

General

Handle null or empty mappings

Guard against null or empty flatMapping returned by traverseAndFlatten. If an index
has no field mappings or the traversal fails, the subsequent resolveBucketMapping
call could produce incorrect results or throw an exception.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [79-89]

 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> flatMapping =
         OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings());
+    if (flatMapping == null || flatMapping.isEmpty()) {
+      excludedIndices.add(entry.getKey());
+      continue;
+    }
     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 adds defensive handling for flatMapping being null or empty. While traverseAndFlatten is unlikely to return null based on the codebase, the check is reasonable defensive programming but has low impact since the scenario is unlikely.

Low

Validate bucket names before planning

Validate that bucketNames is not empty before passing it to
PartialResultAggregatePushdown.plan. An empty list could indicate a malformed
aggregate or edge case that should be handled explicitly rather than relying on
downstream null checks.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [472-479]

 private AbstractRelNode tryPartialResultAggregate(
     Aggregate aggregate, @Nullable Project project) {
   if (!isPartialResultEnabled()) {
     return null;
   }
   if (!QueryContext.isWarningsSupported()) {
     return null;
   }
   try {
     List<String> outputFields = aggregate.getRowType().getFieldNames();
     List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality());
+    if (bucketNames.isEmpty()) {
+      return null;
+    }
     Map<String, IndexMapping> mappings = osIndex.getIndexMappings();
     PartialResultAggregatePushdown.Plan plan =
         PartialResultAggregatePushdown.plan(bucketNames, mappings);
     if (plan == null) {
       return null;
     }

Suggestion importance[1-10]: 3

__

Why: The suggestion adds a redundant check. PartialResultAggregatePushdown.plan already validates bucketNames.isEmpty() at line 72, so checking it again in the caller is unnecessary and adds no value.

Low

Previous suggestions

Suggestions up to commit 8a040a2

Category	Suggestion	Impact
General	Validate kept indices before construction Ensure `plan.keptIndices()` is not empty before constructing the narrowed index. If the kept indices list is empty, `String.join` will produce an empty string, which could lead to an invalid index name and cause query failures downstream. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [486-495] +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)); Suggestion importance[1-10]: 7 __ Why: This is a valid concern. If `plan.keptIndices()` is empty, constructing an `OpenSearchIndex` with an empty string could cause downstream failures. The check should be added before creating the narrowed index to prevent invalid index names from being passed to OpenSearch.	Medium
	Guard against null field mappings Check if `entry.getValue().getFieldMappings()` returns null before calling `traverseAndFlatten`. A null mapping could cause a NullPointerException when attempting to flatten, especially if an index has no defined field mappings. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [79-89] 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()) { - // 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> 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()); default -> excludedIndices.add(entry.getKey()); } } Suggestion importance[1-10]: 5 __ Why: Adding a null check for `fieldMappings` before calling `traverseAndFlatten` is a reasonable defensive practice. However, the `IndexMapping` constructor and usage patterns in the codebase suggest field mappings should not be null, making this a low-probability edge case.	Low
	Validate bucket names before planning Validate that `bucketNames` is not empty before calling `PartialResultAggregatePushdown.plan`. An empty bucket list could occur if `aggregate.getGroupSet().cardinality()` returns 0, leading to an empty sublist that might cause unexpected behavior in the planning logic. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [473-479] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project) { if (!isPartialResultEnabled()) { return null; } - // A partial result is only safe if the response can surface the warning that says so. Refuse - // (fall through to the normal path) for formats without a warnings channel, e.g. CSV/RAW/VIZ. if (!QueryContext.isWarningsSupported()) { return null; } try { List<String> outputFields = aggregate.getRowType().getFieldNames(); List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality()); - // Per-index mappings keyed by concrete index name (the wildcard is already resolved), reused - // from the fetch that resolved this index's field types rather than re-requesting them. + if (bucketNames.isEmpty()) { + return null; + } Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion to validate `bucketNames` is redundant because `PartialResultAggregatePushdown.plan` already checks for empty bucket names at line 72 and returns null. Adding this check earlier would be a minor optimization but doesn't fix a bug.	Low
	Validate thread context removal operation Verify that `ThreadContext.remove` successfully removes the key when `override` is null. If the key doesn't exist, ensure the removal operation doesn't throw an exception or leave the context in an inconsistent state, which could affect subsequent queries on pooled threads. common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java [119-125] public static void setPartialResultOverride(Boolean override) { if (override == null) { - ThreadContext.remove(PARTIAL_RESULT_OVERRIDE_KEY); + String removed = ThreadContext.remove(PARTIAL_RESULT_OVERRIDE_KEY); + if (removed != null && LOG.isDebugEnabled()) { + LOG.debug("Removed partial result override: {}", removed); + } } else { ThreadContext.put(PARTIAL_RESULT_OVERRIDE_KEY, Boolean.toString(override)); } } Suggestion importance[1-10]: 2 __ Why: The suggestion to verify `ThreadContext.remove` success is overly cautious. The `improved_code` adds logging but doesn't address any actual error condition, as `remove` operations on thread-local contexts are safe even when the key doesn't exist. The added logging provides minimal value.	Low

Suggestions up to commit 44e0eaa

Category	Suggestion	Impact
Possible issue	Add null parameter validation The method doesn't validate that `bucketNames` or `mappings` are non-null before checking their size/emptiness. If either parameter is null, this will throw a NullPointerException. Add null checks at the method entry to fail fast with a clear contract. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [71-74] static Plan plan(List<String> bucketNames, Map<String, IndexMapping> mappings) { + if (bucketNames == null \|\| mappings == null) { + return null; + } if (bucketNames.isEmpty() \|\| mappings.size() < 2) { return null; } Suggestion importance[1-10]: 7 __ Why: Adding null checks for `bucketNames` and `mappings` parameters improves robustness and prevents `NullPointerException`. This is a valid defensive programming practice, though the current callers appear to pass non-null values. The suggestion correctly identifies a potential issue and provides appropriate validation.	Medium
	Validate keptIndices is non-empty If `plan.keptIndices()` returns an empty list, `String.join(",", ...)` produces an empty string, which may cause the `OpenSearchIndex` constructor to fail or behave unexpectedly. Validate that `keptIndices` is non-empty before constructing the narrowed index to prevent invalid index patterns. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [486-495] +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); Suggestion importance[1-10]: 6 __ Why: The suggestion to validate that `keptIndices` is non-empty before constructing the narrowed index is reasonable. An empty index pattern could cause issues. However, the logic in `PartialResultAggregatePushdown.plan()` already ensures that `keptIndices` is non-empty (it returns null if no aggregatable subset exists), so this check is somewhat redundant but adds an extra safety layer.	Low
	Add null/empty check for mappings The method retrieves `osIndex.getIndexMappings()` which may be null or empty if the cached mappings haven't been resolved yet. Add a null/empty check before calling `PartialResultAggregatePushdown.plan()` to prevent potential NullPointerException or incorrect behavior when mappings are unavailable. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [476] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project) { if (!isPartialResultEnabled()) { return null; } if (!QueryContext.isWarningsSupported()) { return null; } try { List<String> outputFields = aggregate.getRowType().getFieldNames(); List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality()); 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 if `mappings` is null or empty is reasonable for defensive programming. However, `getIndexMappings()` is called after `resolveFieldOpenSearchTypes()` which initializes `cachedIndexMappings` to a non-null map (either populated or empty `Map.of()`), so a null check is unnecessary. An empty check could be useful but has low impact since `PartialResultAggregatePushdown.plan()` already handles the case where `mappings.size() < 2`.	Low

Suggestions up to commit 755031e

Category	Suggestion	Impact
General	Validate thread context value parsing The `getPartialResultOverride` method uses `Boolean.parseBoolean`, which returns `false` for any non-"true" string (including malformed values). If the thread context is corrupted or contains an invalid value, this silently returns `false` instead of `null`, incorrectly overriding the cluster setting. Validate the stored value or use a more robust parsing approach. common/src/main/java/org/opensearch/sql/common/utils/QueryContext.java [131-134] public static void setPartialResultOverride(Boolean override) { if (override == null) { ThreadContext.remove(PARTIAL_RESULT_OVERRIDE_KEY); } else { ThreadContext.put(PARTIAL_RESULT_OVERRIDE_KEY, Boolean.toString(override)); } } public static Boolean getPartialResultOverride() { String value = ThreadContext.get(PARTIAL_RESULT_OVERRIDE_KEY); - return value == null ? null : Boolean.parseBoolean(value); + if (value == null) { + return null; + } + if ("true".equalsIgnoreCase(value)) { + return true; + } else if ("false".equalsIgnoreCase(value)) { + return false; + } + return null; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `Boolean.parseBoolean` returns `false` for any non-"true" string, which could incorrectly treat corrupted values as explicit `false` overrides instead of `null` (defer to cluster setting). The improved code validates the stored value explicitly, preventing silent misinterpretation of corrupted thread context data.	Medium
General	Handle mapping retrieval failures explicitly The method retrieves index mappings from the client without handling potential network or cluster failures. If `getIndexMappings` throws an exception (e.g., cluster unavailable), the catch block logs it but returns `null`, silently falling back to the normal path. This could mask critical infrastructure issues. Consider propagating exceptions for transient failures or adding explicit error handling. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [477-478] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project) { if (!isPartialResultEnabled()) { return null; } if (!QueryContext.isWarningsSupported()) { return null; } try { List<String> outputFields = aggregate.getRowType().getFieldNames(); List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality()); - Map<String, IndexMapping> mappings = - osIndex.getClient().getIndexMappings(osIndex.getIndexName().getIndexNames()); + Map<String, IndexMapping> mappings; + try { + mappings = osIndex.getClient().getIndexMappings(osIndex.getIndexName().getIndexNames()); + } catch (IOException e) { + LOG.warn("Failed to retrieve index mappings for partial result fallback", e); + return null; + } PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } ... Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that `getIndexMappings` could throw exceptions, but the existing code already has a catch block at line 506 that handles all exceptions. The suggestion adds redundant nested try-catch, which doesn't improve error handling since the outer catch already logs and returns `null`. The concern is valid but the implementation is already adequate.	Low
Possible issue	Validate mapping entries before processing The method does not validate that `mappings` contains non-null values. If `entry.getValue()` returns `null` or `getFieldMappings()` returns `null`, `traverseAndFlatten` could throw a `NullPointerException`. Add a null-check before processing each mapping entry to prevent runtime failures on malformed cluster metadata. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [79-89] 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()) { + IndexMapping mapping = entry.getValue(); + if (mapping == null \|\| mapping.getFieldMappings() == null) { + excludedIndices.add(entry.getKey()); + continue; + } Map<String, OpenSearchDataType> flatMapping = - OpenSearchDataType.traverseAndFlatten(entry.getValue().getFieldMappings()); + OpenSearchDataType.traverseAndFlatten(mapping.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]: 6 __ Why: The suggestion identifies a potential `NullPointerException` if `IndexMapping` or its field mappings are null. While the concern is valid for defensive programming, the code assumes `getIndexMappings` returns well-formed data. Adding null-checks improves robustness against malformed cluster metadata, though such cases should be rare in practice.	Low

Suggestions up to commit 42b4977

Category	Suggestion	Impact
General	Validate kept indices before narrowing Verify that `plan.keptIndices()` is not empty before constructing `narrowedIndex`. An empty kept-indices list would create an invalid index pattern, potentially causing downstream errors or unexpected behavior when the narrowed scan executes. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [486-502] +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; } CalcitePlanContext.addWarning(plan.warning()); return pushed; Suggestion importance[1-10]: 5 __ Why: The suggestion to validate `plan.keptIndices()` is not empty before constructing `narrowedIndex` is valid. While `PartialResultAggregatePushdown.plan` should not return a plan with empty kept indices (lines 98-108), adding this defensive check prevents potential issues if the logic changes or an edge case is missed, improving robustness.	Low
General	Validate non-empty bucket names early Validate that `bucketNames` is not empty before passing it to `PartialResultAggregatePushdown.plan`. An empty group set could lead to unexpected behavior or bypass the intended partial-result logic, even though `plan` itself checks for empty buckets. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [472-480] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project) { if (!isPartialResultEnabled()) { return null; } if (!QueryContext.isWarningsSupported()) { return null; } try { List<String> outputFields = aggregate.getRowType().getFieldNames(); List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality()); + if (bucketNames.isEmpty()) { + return null; + } Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } ... Suggestion importance[1-10]: 3 __ Why: While the suggestion to validate `bucketNames` is not empty is reasonable, `PartialResultAggregatePushdown.plan` already checks for empty bucket names at line 72 and returns `null`. This makes the additional check redundant, though it could provide a minor optimization by avoiding the `getIndexMappings()` call.	Low

Suggestions up to commit 3d21944

Category	Suggestion	Impact
Possible issue	Ensure bucket names extraction is correct The `bucketNames` extraction assumes the first N fields in the row type correspond to the group keys, but this may not hold if the aggregate has a complex projection or reordering. Verify that `aggregate.getGroupSet().cardinality()` correctly identifies the group-by fields, or extract bucket names directly from `aggregate.getGroupSet()` to avoid misalignment. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [473-474] private AbstractRelNode tryPartialResultAggregate( Aggregate aggregate, @Nullable Project project) { if (!isPartialResultEnabled()) { return null; } if (!QueryContext.isWarningsSupported()) { return null; } try { - List<String> outputFields = aggregate.getRowType().getFieldNames(); - List<String> bucketNames = outputFields.subList(0, aggregate.getGroupSet().cardinality()); + List<String> bucketNames = aggregate.getGroupSet().asList().stream() + .map(i -> aggregate.getInput().getRowType().getFieldNames().get(i)) + .collect(Collectors.toList()); Map<String, IndexMapping> mappings = osIndex.getIndexMappings(); PartialResultAggregatePushdown.Plan plan = PartialResultAggregatePushdown.plan(bucketNames, mappings); if (plan == null) { return null; } ... Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue where `bucketNames` extraction via `subList(0, cardinality())` may not align with the actual group-by fields if the aggregate has complex projections. Extracting bucket names directly from `aggregate.getGroupSet()` is more robust and ensures correctness.	Medium
General	Validate kept indices before narrowing The `narrowedIndex` is constructed with a comma-separated list of indices, but there is no validation that `plan.keptIndices()` is non-empty. If the plan somehow returns an empty list, the resulting index pattern would be invalid. Add a check to ensure `keptIndices` is not empty before constructing the narrowed index. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java [483-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)); Suggestion importance[1-10]: 4 __ Why: The suggestion adds a defensive check for an empty `keptIndices` list before constructing the narrowed index. While this is good defensive programming, the `plan` method already ensures `keptIndices` is non-empty (lines 98-104 in `PartialResultAggregatePushdown.java`), so this scenario should not occur. The check adds minor safety but is largely redundant.	Low
General	Handle empty bucket names list The method does not handle the case where `bucketNames` is empty. An empty list would return `KEYWORD` by default, which may not be semantically correct. Add an explicit check to return a meaningful resolution or throw an exception when `bucketNames` is empty. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/PartialResultAggregatePushdown.java [121-138] static MappingResolution resolveBucketMapping( Map<String, OpenSearchDataType> flatMapping, List<String> bucketNames) { + if (bucketNames.isEmpty()) { + return MappingResolution.NOT_AGGREGATABLE; + } 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) { continue; } else if (hasKeywordSubField(type)) { combined = MappingResolution.TEXT_WITH_KEYWORD; } else { return MappingResolution.NOT_AGGREGATABLE; } } return combined; } Suggestion importance[1-10]: 3 __ Why: The suggestion addresses a minor edge case where an empty `bucketNames` list would return `KEYWORD` by default. However, the caller (`plan` method) already checks for empty `bucketNames` at line 72, so this scenario is already guarded against. The suggestion is valid but has low impact.	Low

A wide observability pattern can exclude hundreds of indices; listing them all verbatim in the warning detail produces an unreadable multi-kilobyte message. The exact count is already in the warning's summary, so spell out at most a few excluded index names in the detail and summarize the rest as "... and N more". Add an integration test with a large excluded set asserting the detail is truncated while the summary still reports the full count. Signed-off-by: Kai Huang <ahkcs@amazon.com>

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

Persistent review updated to latest commit dad3bb3

The detail said the aggregation ran over the 'largest consistently-mapped subset', leftover from when the kept group was chosen by index count. Selection is now by whether the field is aggregatable there (keyword-first), not size, so reword to 'ran only over the indices where the field is aggregatable'. Signed-off-by: Kai Huang <ahkcs@amazon.com>

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

Persistent review updated to latest commit 078c949

Harden the partial-result fallback from POC-shaped code into a testable unit: - Move the classify/partition/priority/warning logic out of the 600-line CalciteLogicalIndexScan into a dedicated PartialResultAggregatePushdown. The scan's tryPartialResultAggregate keeps only the plan-time wiring (settings gate, mapping lookup, narrowed-scan construction, warning emission) and delegates the decision to PartialResultAggregatePushdown.plan(...). - Make the PARTIAL_RESULT warning type a shared constant (Warning.TYPE_PARTIAL_RESULT) instead of a literal, since consumers such as OpenSearch Dashboards branch on it -- a cross-surface contract. - Add a field-map constructor to IndexMapping for testability. - Add unit tests covering classification (keyword / text+keyword / bare-text / absent), multi-field weakest-resolution, the keyword-first priority ladder (including when keyword is outnumbered), null/no-op cases, excluded-list sorting, and warning-list truncation. No behavior change; the integration tests are unchanged and still pass. Signed-off-by: Kai Huang <ahkcs@amazon.com>

Partial-result mode was gated solely by the cluster setting. Add an optional per-request partial_result flag (e.g. from an OpenSearch Dashboards toggle) that takes precedence when present: true forces partial mode on for that query, false forces it off, and an absent flag defers to the cluster setting. - Parse partial_result from the PPL request body into a nullable Boolean on PPLQueryRequest / TransportPPLQueryRequest (mirrors the profile flag; null means 'unset'). - Carry it into QueryContext as a per-request override, cleared each request so it cannot leak across pooled worker threads. - The producer gate now resolves override != null ? override : clusterSetting. Signed-off-by: Kai Huang <ahkcs@amazon.com>

…-result-warning-channel # Conflicts: # core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java

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

Persistent review updated to latest commit 83fd527

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>

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

Persistent review updated to latest commit 2a3eab8

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>

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

…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>

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: Return a partial result with a warning instead of exhausting PIT contexts on a text/keyword mapping conflict #5657 (comment)

penghuo · 2026-07-29T12:38:55Z

        "plugins.calcite.pushdown.rowcount.estimation.factor"),
    CALCITE_SUPPORT_ALL_JOIN_TYPES("plugins.calcite.all_join_types.allowed"),
+    CALCITE_PARTIAL_RESULT_ON_MAPPING_CONFLICT(
+        "plugins.calcite.partial_result.on_mapping_conflict.enabled"),


calcite is internal impl. plugins.query.partial_result.on_mapping_conflict.enabled is better.

We should have multiple settings?

Does mapping conflict allowed?

Yes (default, PPL current behavior). I suspect text/keyword conflict not the only case.

No: Fast Fail.

Does aggregation on text field allowed?

Yes: Use script text aggregation.

No: There are multiple choice. Ignore index has text field or Fast Fail or use null value instead of fetch from text field.

Thanks for the suggestion, updated the naming

Current behavior isn't uniform

MergeRuleHelper tries three rules, first match wins:

Conflict Rule Today

STRUCT/ARRAY, same core type DeepMergeRule real merge, correct

text ↔ keyword TextKeywordConflictRule collapses to text-without-.keyword → no doc values → no pushdown → PIT per shard → 500 past the limit

everything else (long↔integer, ip↔keyword, date↔long…) LatestRule target.put(key, source) while iterating a toUnmodifiableMap — the last mapping yielded wins, and that order isn't guaranteed

So you're right that text/keyword isn't the only case. For the rest, the merged type is arbitrary per index set — stable for a given set, but two identically-shaped sets can disagree. Verified on 12 freshly-created, identical long+integer conflicts:

describe → 10 sets resolved to `int`, 2 to `bigint` docs 5000000000 + 7: int sets → [7, 705032704] ← 64-bit value truncated, 200 OK, no warning bigint sets → [7, 5000000000] ← correct

long+integer → int is narrowing, so this isn't least-restrictive widening. Filtering is honored correctly, so it's a type-resolution defect, not a predicate bug. Its own issue, independent of this PR.

Settings

allow needs defining. "Current behavior" means three different things above. It should mean merge and warn, not "merge and sometimes silently truncate" — making this PR's warning channel the substrate for the whole matrix. Fast-fail is genuinely useful: for a LatestRule conflict, failing loudly beats silent truncation.

Proposal: make it discoverable, not a flag users must know

Backend analyze — detect the failed-pushdown signature (aggregate above the scan, requestedTotalSize=2147483647) and emit a recommendation naming the field, the conflict, and the remedy. AnalyzeResponse already has a recommendations field, so this is additive.

Frontend inspect query — render it with the honest trade-off (faster, no PIT, partial).

Scope

This PR: warning channel + PARTIAL_RESULT producer + single opt-in boolean (default off, inert)

Follow-ups: (1) analyze recommendation + inspect query; (2) LatestRule arbitrary-type/truncation bug;

penghuo · 2026-07-29T12:54:22Z

+      // Normal pushdown failed (e.g. a text/keyword mapping conflict across a wildcard pattern that
+      // would otherwise fall back to a per-shard scan and exhaust PIT contexts). If partial results
+      // are enabled, push the aggregation over the aggregatable index subset instead and warn.
+      newRelNode = scan.tryPartialResultAggregate(aggregate, project);


why add a new function instead of reuse pushDownAggregate?

Updated, rule is back to one call

penghuo · 2026-07-29T13:04:56Z

+      Map<String, IndexMapping> mappings =
+          osIndex.getClient().getIndexMappings(osIndex.getIndexName().getIndexNames());


avoid call indexMapping again.

Updated — now osIndex.getIndexMappings(), reusing the fetch getFieldTypes() already makes. No second call.

…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>

github-actions · 2026-07-29T18:49:54Z

Persistent review updated to latest commit a996d24

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>

github-actions · 2026-07-29T19:03:32Z

Persistent review updated to latest commit 3d21944

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>

github-actions · 2026-07-29T19:19:00Z

Persistent review updated to latest commit 42b4977

…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>

github-actions · 2026-07-29T21:11:27Z

Persistent review updated to latest commit 755031e

… 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>

github-actions · 2026-07-30T17:19:16Z

Persistent review updated to latest commit 44e0eaa

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>

github-actions · 2026-07-30T19:10:25Z

Persistent review updated to latest commit 8a040a2

…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>

github-actions · 2026-07-30T19:27:11Z

Persistent review updated to latest commit cdfab96

ahkcs added 5 commits July 27, 2026 11:46

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

ahkcs added 3 commits July 27, 2026 16:06

Merge remote-tracking branch 'upstream/main' into feature/ppl-partial…

83fd527

…-result-warning-channel # Conflicts: # core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java

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

ahkcs requested review from Swiddis, dai-chen, joshuali925, mengweieric, ps48 and vamsimanohar as code owners July 28, 2026 21:26

ahkcs requested review from LantaoJin, RyanL1997, acarbonetto, anirudha, noCharger, penghuo, qianheng-aws, songkant-aws, ykmr1224 and yuancu as code owners July 28, 2026 21:26

penghuo reviewed Jul 29, 2026

View reviewed changes

Fix formatting of the renamed partial-result setting key

42b4977

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>

Conflict	Rule	Today
STRUCT/ARRAY, same core type	`DeepMergeRule`	real merge, correct
text ↔ keyword	`TextKeywordConflictRule`	collapses to `text`-without-`.keyword` → no doc values → no pushdown → PIT per shard → 500 past the limit
everything else (long↔integer, ip↔keyword, date↔long…)	`LatestRule`	`target.put(key, source)` while iterating a `toUnmodifiableMap` — the last mapping yielded wins, and that order isn't guaranteed

		Map<String, IndexMapping> mappings =
		osIndex.getClient().getIndexMappings(osIndex.getIndexName().getIndexNames());

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

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 cdfab96)

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!

penghuo Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahkcs Jul 29, 2026

Choose a reason for hiding this comment

Current behavior isn't uniform

Settings

Proposal: make it discoverable, not a flag users must know

Scope

Uh oh!

penghuo Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

ahkcs Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

penghuo Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

ahkcs Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026

ahkcs commented Jul 27, 2026 •

edited

Loading

github-actions Bot commented Jul 27, 2026 •

edited

Loading

(Review updated until commit `cdfab96`)

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)

penghuo Jul 29, 2026 •

edited

Loading