Push down aggregation on text field without .keyword sub-field by penghuo · Pull Request #5646 · opensearch-project/sql

penghuo · 2026-07-22T22:57:03Z

Description

When a text field has no .keyword sub-field, PPL queries that group by or aggregate over that field silently fall back to a full _source scan with client-side aggregation, even when Calcite pushdown is enabled.

Fix. When getReferenceForTermQuery() returns null for a bare RexInputRef, route through ScriptQueryExpression(rexNode, ...) to build a Calcite script that reads the field from _source. This reuses the exact SOURCE path already established by TermQuery/LikeQuery/RexStandardizer.visitInputRef and consumed at runtime by CalciteScriptEngine.ScriptDataContext.getFromSource → SourceLookup.get(...). The change is in AggregateAnalyzer.AggregateBuilderHelper.build — one branch added.

Related Issues

Resolves #5634

Check List

New functionality includes testing.
- Unit tests in AggregateAnalyzerTest: analyze_aggCall_TextWithoutKeyword_countPushesDownAsScript, analyze_groupBy_TextWithoutKeyword (asserts scripted value_count / terms DSL and SOURCES/DIGESTS script params).
- YAML rest tests in text_agg_pushdown.yml (10 cases) covering top, stats … by, count(field) — including result correctness and pushed-down DSL assertions — plus baselines for text+keyword and multi-index queries.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
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.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

For a text-typed group key or metric argument with no .keyword sub-field, NamedFieldExpression.getReferenceForTermQuery() returned null and CompositeValuesSourceBuilder/ValueCountAggregationBuilder rejected the null field, so pushDownAggregate silently fell back to a full _source scan and client-side aggregation. Route those bare RexInputRefs through a Calcite script that reads the value from _source, matching TermQuery/LikeQuery/RexStandardizer for filter and script fields. Composite terms buckets and metric aggregations that accept a script (notably count(FIELD)) now push down. Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

github-actions · 2026-07-22T22:58:15Z

PR Reviewer Guide 🔍

(Review updated until commit `16ed19b`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

github-actions · 2026-07-22T22:58:31Z

PR Code Suggestions ✨

Latest suggestions up to 16ed19b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle empty string from field reference The null check on `fieldRef` assumes `getReferenceForTermQuery()` returns null for text fields without a keyword sub-field. However, if this method throws an exception or returns an empty string instead, the fallback logic will not trigger correctly. Add explicit validation or document the expected contract of `getReferenceForTermQuery()`. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [163-169] String fieldRef = inferNamedField(node).getReferenceForTermQuery(); // Text field with no .keyword sub-field is not aggregatable directly. Fall back to a // Calcite script that reads the value from _source. -if (fieldRef == null) { +if (fieldRef == null \|\| fieldRef.isEmpty()) { return scriptBuilder.apply(inferScript(node).getScript()); } return fieldBuilder.apply(fieldRef); Suggestion importance[1-10]: 3 __ Why: The suggestion to check for empty string is a defensive programming practice, but there's no evidence in the PR that `getReferenceForTermQuery()` returns empty strings. The existing null check appears sufficient based on the test cases and implementation context. The suggestion adds marginal value without clear justification.	Low

Previous suggestions

Suggestions up to commit 99d01c3

Category	Suggestion	Impact
General	Document RexInputRef script handling Adding `RexInputRef` to the script inference path is a significant behavioral change. Ensure that `ScriptQueryExpression` correctly handles `RexInputRef` nodes, as they represent direct field references and may require special handling compared to expressions (`RexCall`) or literals (`RexLiteral`). opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [184-187] ScriptQueryExpression inferScript(RexNode node) { if (node instanceof RexCall \|\| node instanceof RexLiteral \|\| node instanceof RexInputRef) { + // RexInputRef added to support text fields without .keyword via _source script return new ScriptQueryExpression( node, rowType, fieldTypes, cluster, Collections.emptyMap()); } Suggestion importance[1-10]: 3 __ Why: Adding a comment to document why `RexInputRef` was added to the script inference path would improve code maintainability. However, this is a minor documentation improvement that doesn't affect functionality.	Low

Suggestions up to commit c409ddf

Category	Suggestion	Impact
General	Handle empty string from field reference The null check on `fieldRef` assumes `getReferenceForTermQuery()` returns null for text fields without a keyword sub-field. However, if this method throws an exception or returns an empty string instead, the fallback logic will fail. Add explicit validation or exception handling to ensure robustness. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [161-167] String fieldRef = inferNamedField(node).getReferenceForTermQuery(); // Text field with no .keyword sub-field is not aggregatable directly. Fall back to a // Calcite script that reads the value from _source. -if (fieldRef == null) { +if (fieldRef == null \|\| fieldRef.isEmpty()) { return scriptBuilder.apply(inferScript(node).getScript()); } return fieldBuilder.apply(fieldRef); Suggestion importance[1-10]: 5 __ Why: The suggestion to check for empty strings in addition to null is a defensive programming practice that could prevent edge cases. However, the PR context shows this is specifically handling text fields without `.keyword` sub-fields where `getReferenceForTermQuery()` returns null. Without evidence that empty strings are returned in practice, this is a minor defensive improvement rather than a critical fix.	Low

Suggestions up to commit 91204e9

Category	Suggestion	Impact
General	Handle empty string from field reference The null check on `fieldRef` assumes `getReferenceForTermQuery()` returns null for text fields without a keyword sub-field. However, if this method throws an exception or returns an empty string instead, the fallback logic will not trigger correctly. Add explicit validation or document the expected behavior of `getReferenceForTermQuery()` to ensure robustness. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [161-167] String fieldRef = inferNamedField(node).getReferenceForTermQuery(); // Text field with no .keyword sub-field is not aggregatable directly. Fall back to a // Calcite script that reads the value from _source. -if (fieldRef == null) { +if (fieldRef == null \|\| fieldRef.isEmpty()) { return scriptBuilder.apply(inferScript(node).getScript()); } return fieldBuilder.apply(fieldRef); Suggestion importance[1-10]: 5 __ Why: The suggestion to check for empty strings in addition to null is a defensive programming practice that could prevent potential issues. However, without evidence that `getReferenceForTermQuery()` can return empty strings, this is a minor improvement. The PR's implementation already handles the null case correctly for the stated purpose of handling text fields without `.keyword` sub-fields.	Low

Suggestions up to commit 70c263d

Category	Suggestion	Impact
General	Add logging for script fallback The null check for `fieldRef` may silently mask unexpected null values from fields that should be aggregatable. Consider logging a debug or trace message when falling back to script-based aggregation to aid troubleshooting and ensure the fallback is intentional for text-only fields. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [161-167] String fieldRef = inferNamedField(node).getReferenceForTermQuery(); // Text field with no .keyword sub-field is not aggregatable directly. Fall back to a // Calcite script that reads the value from _source. if (fieldRef == null) { + log.debug("Field reference is null for node {}, falling back to script-based aggregation", node); return scriptBuilder.apply(inferScript(node).getScript()); } return fieldBuilder.apply(fieldRef); Suggestion importance[1-10]: 4 __ Why: Adding debug logging when falling back to script-based aggregation could help with troubleshooting, but this is a minor enhancement. The null check is intentional for text fields without `.keyword` sub-fields, as documented in the comment and validated by the extensive test coverage in the PR.	Low

Suggestions up to commit 1760521

Category	Suggestion	Impact
General	Add null safety for field reference The null check for `fieldRef` may not be sufficient to detect all non-aggregatable text fields. Consider verifying that `inferNamedField(node)` itself doesn't return null before calling `getReferenceForTermQuery()` to prevent potential NullPointerException. Add defensive null handling for the `NamedFieldExpression` object. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [161-167] -String fieldRef = inferNamedField(node).getReferenceForTermQuery(); +NamedFieldExpression namedField = inferNamedField(node); +String fieldRef = namedField.getReferenceForTermQuery(); // Text field with no .keyword sub-field is not aggregatable directly. Fall back to a // Calcite script that reads the value from _source. if (fieldRef == null) { return scriptBuilder.apply(inferScript(node).getScript()); } return fieldBuilder.apply(fieldRef); Suggestion importance[1-10]: 3 __ Why: The suggestion adds an intermediate variable to store the result of `inferNamedField(node)`, but this doesn't add meaningful null safety since `inferNamedField` throws an `IllegalStateException` rather than returning null (as seen in lines 179-180). The refactoring provides minimal value and the concern about NPE is unfounded given the implementation.	Low

Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

github-actions · 2026-07-22T23:08:19Z

Persistent review updated to latest commit 1760521

Four CalciteExplainIT plans encoded the pre-fix behavior where dedup / chart / timechart on a text field with no .keyword sub-field silently fell back to a client-side aggregation over an unbounded scan. With the AggregateAnalyzer fix those queries now push down as composite terms (script over _source), so the pinned physical plans are stale. - Rename testDedupTextTypeNotPushdown -> testDedupTextTypePushdown and update explain_dedup_text_type_push.yaml to the composite terms + top_hits DSL. - Refresh chart_null_str.yaml (chart limit=10 ... over gender by age span=10) to the composite terms(script) + histogram plan. - Refresh explain_timechart.yaml and explain_timechart_count.yaml (timechart span=1m ... by host) to the composite terms(script) + date_histogram plan. Add DedupCommandIT.testDedupOnTextField to verify behavioral equivalence: the result set for `source=bank | dedup email` matches the fixture's set of distinct emails, running both under the V2 path (base class) and Calcite pushdown path (CalciteDedupCommandIT). Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

github-actions · 2026-07-23T16:55:34Z

PR Code Analyzer ❗

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

Path	Line	Severity	Description
integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_push.yaml	9	low	Multiple test fixtures and expected-output YAML files embed Base64 strings beginning with 'rO0ABX', which is the Java serialization magic header (0xACED0005). The decoded content is a JSON metadata object describing a VARCHAR type parameter, consistent with the existing opensearch_compounded_script mechanism. This pattern was already present in the codebase and the new change extends it to text-field aggregation pushdown paths. No malicious content detected in the decoded payload, but reviewers should confirm the OpenSearch-side deserializer is constrained to safe types to prevent gadget-chain exploitation if script sources are ever influenced by untrusted input.

The table above displays the top 10 most important findings.

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

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-23T16:56:14Z

Persistent review updated to latest commit 70c263d

Widen the assertion beyond the dedup key to also verify the associated projected columns per row (firstname, balance), so the top_hits round-trip in the pushed-down dedup DSL is checked end-to-end. Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

github-actions · 2026-07-23T17:53:21Z

Persistent review updated to latest commit 91204e9

Assert row-level results for `source=events | timechart span=1m avg(cpu_usage) by host` on the events fixture, where `host` is a text field with no .keyword sub-field. Golden values were collected on upstream/main (unpushed) before applying the fix, so the assertion pins behavioral equivalence between the V2 client-side plan and the pushed composite terms(script)+date_histogram plan. Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

github-actions · 2026-07-23T21:21:29Z

Persistent review updated to latest commit c409ddf

Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo · 2026-07-23T20:40:53Z

-                    EnumerableCalc(expr#0..2=[{inputs}], expr#3=[10], expr#4=[null:NULL], expr#5=[SPAN($t2, $t3, $t4)], gender=[$t1], balance=[$t0], age0=[$t5])
-                      CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank_with_null_values]], PushDownContext=[[PROJECT->[balance, gender, age], FILTER->AND(IS NOT NULL($1), IS NOT NULL($0))], OpenSearchRequestBuilder(sourceBuilder={"from":0,"timeout":"1m","query":{"bool":{"must":[{"exists":{"field":"gender","boost":1.0}},{"exists":{"field":"balance","boost":1.0}}],"adjust_pure_negative":true,"boost":1.0}},"_source":{"includes":["balance","gender","age"]}}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])
+                EnumerableCalc(expr#0..2=[{inputs}], expr#3=[SAFE_CAST($t1)], gender=[$t0], age=[$t3], avg(balance)=[$t2])
+                  CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank_with_null_values]], PushDownContext=[[FILTER->AND(IS NOT NULL($1), IS NOT NULL($0)), AGGREGATION->rel#:LogicalAggregate.NONE.[](input=RelSubset#,group={0, 2},avg(balance)=AVG($1))], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":0,"timeout":"1m","query":{"bool":{"must":[{"exists":{"field":"gender","boost":1.0}},{"exists":{"field":"balance","boost":1.0}}],"adjust_pure_negative":true,"boost":1.0}},"aggregations":{"composite_buckets":{"composite":{"size":1000,"sources":[{"gender":{"terms":{"script":{"source":"{\"langType\":\"calcite\",\"script\":\"rO0ABXQAaXsKICAiZHluYW1pY1BhcmFtIjogMCwKICAidHlwZSI6IHsKICAgICJ0eXBlIjogIlZBUkNIQVIiLAogICAgIm51bGxhYmxlIjogdHJ1ZSwKICAgICJwcmVjaXNpb24iOiAtMQogIH0KfQ==\"}","lang":"opensearch_compounded_script","params":{"utcTimestamp": 0,"SOURCES":[1],"DIGESTS":["gender"]}},"missing_bucket":true,"missing_order":"first","order":"asc"}}},{"age0":{"histogram":{"field":"age","missing_bucket":true,"missing_order":"first","order":"asc","interval":10.0}}}]},"aggregations":{"avg(balance)":{"avg":{"field":"balance"}}}}}}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])


EnumerableAggregate is pushed down as script.

github-actions · 2026-07-23T21:26:55Z

Persistent review updated to latest commit 99d01c3

penghuo · 2026-07-30T15:00:25Z

@qianheng-aws Please take a look PR.

# Conflicts: # integ-test/src/test/resources/expectedOutput/calcite/chart_null_str.yaml

github-actions · 2026-07-30T15:12:14Z

Persistent review updated to latest commit 16ed19b

qianheng-aws

LGTM

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>

…pping conflicts (#5657) * Add non-fatal warning channel to PPL query response 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> * Return a partial result instead of exhausting PIT on a mapping conflict 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> * Gate partial results on a warning-capable response format 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> * Flatten per-index mappings when partitioning for partial results 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> * Refine partial-result index selection and warning wording 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> * Truncate the excluded-index list in the partial-result warning 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> * Fix partial-result warning wording to reflect the pushdown criterion 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> * Extract partial-result partitioning into its own class with unit tests 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> * Allow a per-request override for partial-result mode 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> * Simplify partial-result warning to name the excluded indices and the fix 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> * Do not resolve response format for explain requests 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> * Cover the null-warnings branch in QueryResult to satisfy protocol coverage 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> * Address review: rename the partial-result setting and fold the fallback 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> * Reuse the already-fetched index mappings for partial-result partitioning 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> * Fix formatting of the renamed partial-result setting key 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> * Revert the index-mapping reuse optimization: it exposed a merge-mutation 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> * Reuse the already-fetched index mappings, and stop the merge mutating 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> * Clear the per-request partial-result state after each query 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> * Update explain golden files for the changed OpenSearchDataType serialVersionUID 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> * Decide partial-result mode before pushdown analysis, not after it fails 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. #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 #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 #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 #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> * Document the partial-result-on-mapping-conflict setting 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> * Tighten inline comments on the partial-result path Signed-off-by: Kai Huang <ahkcs@amazon.com> * Address review: mark setting experimental, consolidate enable check - 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> * Drop the redundant post-failure partial-result fallbacks 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 #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> * Tighten inline comments Signed-off-by: Kai Huang <ahkcs@amazon.com> * Bail on a non-text type conflict instead of excluding the index 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> * Partition partial results on the group key's source fields 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> * Add IT for a multi-field expression group key 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> * Handle a non-text type vs text conflict in partial results 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 (#5610). Signed-off-by: Kai Huang <ahkcs@amazon.com> * Revert non-text type generalization; scope to text/keyword collapse 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, #5610). Expression-key tracing (#cbf50748) is unaffected and retained. This reverts commit cafac5f. Signed-off-by: Kai Huang <ahkcs@amazon.com> * Generalize partial results to any non-aggregatable-vs-aggregatable conflict 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 (#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> * Address review: drop the special keyword token; sort only when building the warning - resolveBucketSignature: keyword uses the same t:TYPE token as other aggregatable types (no separate 'kw'). - plan(): stop sorting excludedIndices; sort a copy inside buildWarning, since ordering only matters for a readable message. Signed-off-by: Kai Huang <ahkcs@amazon.com> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com>

penghuo added bugFix PPL Piped processing language labels Jul 22, 2026

Apply spotless formatting

1760521

Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo marked this pull request as ready for review July 23, 2026 20:03

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

penghuo marked this pull request as draft July 23, 2026 20:04

Trim comment on testTimechartAvgByTextHost

99d01c3

Signed-off-by: Peng Huo <penghuo@amazon.com> Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo commented Jul 23, 2026

View reviewed changes

penghuo marked this pull request as ready for review July 23, 2026 22:19

ahkcs mentioned this pull request Jul 29, 2026

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

Merged

6 tasks

Merge remote-tracking branch 'upstream/main' into bugFix/5631

16ed19b

# Conflicts: # integ-test/src/test/resources/expectedOutput/calcite/chart_null_str.yaml

qianheng-aws approved these changes Jul 31, 2026

View reviewed changes

qianheng-aws merged commit d5a182d into opensearch-project:main Jul 31, 2026
40 of 42 checks passed

dai-chen mentioned this pull request Aug 13, 2026

[FEATURE] Improve PIT usage for large queries over wildcard index patterns #5698

Open

ryan-gh-bot mentioned this pull request Sep 2, 2026

[AUTOCUT] Integration Test Failed for sql-3.9.0 #5739

Closed

ahkcs mentioned this pull request Sep 9, 2026

Resolve an object-vs-scalar mapping conflict to the scalar #5756

Draft

8 tasks

Navigation Menu

Uh oh!

Push down aggregation on text field without .keyword sub-field - #5646

Push down aggregation on text field without .keyword sub-field#5646
qianheng-aws merged 7 commits into
opensearch-project:mainfrom
penghuo:bugFix/5631

penghuo commented Jul 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

penghuo Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

penghuo commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

qianheng-aws left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

penghuo commented Jul 22, 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 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 16ed19b)

Uh oh!

github-actions Bot commented Jul 22, 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 22, 2026

Uh oh!

github-actions Bot commented Jul 23, 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 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

penghuo Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

penghuo commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

qianheng-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

penghuo commented Jul 22, 2026 •

edited

Loading

github-actions Bot commented Jul 22, 2026 •

edited

Loading

(Review updated until commit `16ed19b`)

github-actions Bot commented Jul 22, 2026 •

edited

Loading

github-actions Bot commented Jul 23, 2026 •

edited

Loading