Skip to content

PPL - Fix count/dc(field) failing after a fields command - #5783

Open
mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:count-filter-frame-fix
Open

mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:count-filter-frame-fix

Conversation

@mch2

@mch2 mch2 commented Sep 18, 2026

Copy link
Copy Markdown
Member

A query likesource=t | fields status | stats count(status) fails with "RexInputRef index 6 out of range 0..0". dc(status) fails the same way. The trigger is a fields that narrows the row in front of an ungrouped count(<field>) or dc(<field>); count() with no argument, every other aggregate, and anything with a by clause are unaffected.

count(field) counts non-null values, so an IS NOT NULL filter is added under the aggregate, that is what lets OpenSearch answer the whole query as a document count instead of an aggregation. The filter is stacked on the project node, so its reference must be the column's index in the project's output, but the index used was the column's index in the project's input: after fields status, status is $0 above the projection and $6 below it, and $6 is past the end of the row the filter sees.

The fix takes the reference from refsPerCount, which holds output-side indices.

I had to make a change to a test golden, the query (eval name = lastname | stats count(name)) widens the row, so the old index was in range and happened to hold the same value. That query was correct before and is correct now, and only the printed reference moves, from the aliased source column to the column being counted.

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • 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.

`source=t | fields status | stats count(status)` never runs — it fails to
plan with "RexInputRef index 6 out of range 0..0". `dc(status)` fails the
same way. The trigger is a `fields` that narrows the row in front of an
ungrouped `count(<field>)` or `dc(<field>)`; `count()` with no argument,
every other aggregate, and anything with a `by` clause are unaffected.

count(field) counts non-null values, so an IS NOT NULL filter is added
under the aggregate — that is what lets OpenSearch answer the whole query
as a document count instead of an aggregation. The filter is stacked on
the projection, so its reference must be the column's index in the
projection's output, but the index used was the column's index in the
projection's input: after `fields status`, status is $0 above the
projection and $6 below it, and $6 is past the end of the row the filter
sees.

The fix takes the reference from refsPerCount, which already holds
output-side indices. Nothing is removed: the input-side mapping still
runs on the line above, where it does the job it was added for — proving
two names are one column, so `count(a), count(alias_of_a)` adds one
filter rather than two.

The explain golden file changes with it. Its query
(`eval name = lastname | stats count(name)`) widens the row, so the old
index was in range and happened to hold the same value — that query was
correct before and is correct now, and only the printed reference moves,
from the aliased source column to the column being counted.

Signed-off-by: Marc Handalian <handalm@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add descriptive exception message

The orElseThrow() call will throw a NoSuchElementException without a descriptive
message if the stream is empty. This could happen if refsPerCount contains only
empty lists. Add a descriptive error message to aid debugging when this unexpected
condition occurs.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1628-1629]

 RexInputRef filterRef =
-    refsPerCount.stream().flatMap(List::stream).findFirst().orElseThrow();
+    refsPerCount.stream().flatMap(List::stream).findFirst()
+        .orElseThrow(() -> new IllegalStateException(
+            "Expected at least one reference in refsPerCount for count aggregation"));
Suggestion importance[1-10]: 4

__

Why: While adding a descriptive exception message is a good practice, the condition is already guarded by refsPerCount.stream().noneMatch(List::isEmpty) in line 1618, making the empty stream case impossible. The suggestion improves error handling but addresses an unlikely scenario.

Low

@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.86%. Comparing base (a1293ee) to head (bee82cf).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../opensearch/sql/calcite/CalciteRelNodeVisitor.java 0.00% 3 Missing ⚠️

❌ Your project check has failed because the head coverage (62.86%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5783      +/-   ##
============================================
+ Coverage     62.84%   62.86%   +0.01%     
- Complexity     8777     8814      +37     
============================================
  Files           937      938       +1     
  Lines         40118    40207      +89     
  Branches       4516     4528      +12     
============================================
+ Hits          25212    25275      +63     
- Misses        14106    14127      +21     
- Partials        800      805       +5     
Flag Coverage Δ
sql-engine 62.86% <0.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants