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

[penghuo]

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 99d01c3)

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]

PR Code Suggestions ✨

Latest suggestions up to 99d01c3

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit bcd0f07

Category	Suggestion	Impact
General	Handle empty string from field reference The null check for fieldRef assumes getReferenceForTermQuery() returns null for non-aggregatable text fields. However, if this method throws an exception or returns an empty string instead, the fallback logic won't trigger. Add explicit validation or document the contract that getReferenceForTermQuery() must return null 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) { +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 strings is a defensive programming practice, but there's no evidence in the PR that getReferenceForTermQuery() returns empty strings. The extensive test coverage in the PR validates the null-check behavior works correctly for text-only fields. This is a minor robustness improvement without clear necessity.	Low

[github-actions]

Persistent review updated to latest commit 1760521

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
integ-test/src/test/resources/expectedOutput/calcite/explain_dedup_text_type_push.yaml	10	medium	Base64-encoded Java-serialized payloads (rO0ABX… prefix = Java serialization magic bytes) appear in OpenSearch script sources throughout the new and updated YAML test fixtures. While these payloads decode to benign Calcite type metadata ({"dynamicParam":0,"type":{"type":"VARCHAR",...}}) and the pattern is consistent with the existing plugin's opensearch_compounded_script mechanism, Java deserialization of attacker-controlled data is a historically exploited attack vector. Maintainers should confirm the serialized content cannot be substituted with a gadget-chain payload at runtime and that deserialization is constrained to known-safe types.

The table above displays the top 10 most important findings.

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

---

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

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

---

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

Thanks.

[github-actions]

Persistent review updated to latest commit 70c263d

[github-actions]

Persistent review updated to latest commit 91204e9

[github-actions]

Persistent review updated to latest commit c409ddf

[penghuo]

EnumerableAggregate is pushed down as script.

[github-actions]

Persistent review updated to latest commit 99d01c3