From bee82cfa7419b2b209aa5b3dd0a4bd1a14ec9136 Mon Sep 17 00:00:00 2001 From: Marc Handalian Date: Thu, 17 Sep 2026 22:39:38 +0000 Subject: [PATCH 1/2] Fix count/dc(field) failing after a fields command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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()` or `dc()`; `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 --- .../sql/calcite/CalciteRelNodeVisitor.java | 13 +++- .../calcite/explain_count_agg_push3.yaml | 4 +- .../ppl/calcite/CalcitePPLCountFrameTest.java | 76 +++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index a2f0addbfb6..003211d60d2 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -1616,7 +1616,18 @@ private Pair, List> aggregateWithTrimming( distinctRefsOfCounts = refsPerCount.stream().flatMap(List::stream).distinct().toList(); } if (distinctRefsOfCounts.size() == 1 && refsPerCount.stream().noneMatch(List::isEmpty)) { - context.relBuilder.filter(context.relBuilder.isNotNull(distinctRefsOfCounts.getFirst())); + // This filter is stacked on top of the Project, so its reference has to address the + // Project's OUTPUT. distinctRefsOfCounts may instead hold an index mapped through the + // Project, which addresses its INPUT — that mapping exists only to prove two aliases are + // one column, and it is not a reference we can use here. `fields COMM | stats count(COMM)` + // is the clearest case: COMM is $6 of the scan and $0 of the projection, and filtering on + // $6 fails with "RexInputRef index 6 out of range 0..0". Where the projection merely + // reorders, the index stays in range and names a different column, so the wrong column's + // nullness is tested. refsPerCount is already in the output frame, and by this branch every + // entry denotes the same column, so any one of them gives the same predicate. + RexInputRef filterRef = + refsPerCount.stream().flatMap(List::stream).findFirst().orElseThrow(); + context.relBuilder.filter(context.relBuilder.isNotNull(filterRef)); } } diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_count_agg_push3.yaml b/integ-test/src/test/resources/expectedOutput/calcite/explain_count_agg_push3.yaml index bd1fff2449a..0e119e2b819 100644 --- a/integ-test/src/test/resources/expectedOutput/calcite/explain_count_agg_push3.yaml +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_count_agg_push3.yaml @@ -3,8 +3,8 @@ calcite: LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) LogicalAggregate(group=[{}], cnt=[COUNT($0)]) LogicalProject(name=[$17]) - LogicalFilter(condition=[IS NOT NULL($10)]) + LogicalFilter(condition=[IS NOT NULL($17)]) LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10], _id=[$11], _index=[$12], _score=[$13], _maxscore=[$14], _sort=[$15], _routing=[$16], name=[$10]) CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]]) physical: | - CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[FILTER->IS NOT NULL($0), AGGREGATION->rel#:LogicalAggregate.NONE.[](input=RelSubset#,group={},cnt=COUNT($1)), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":0,"timeout":"1m","query":{"exists":{"field":"lastname","boost":1.0}},"track_total_hits":2147483647}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)]) + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[FILTER->IS NOT NULL($0), AGGREGATION->rel#:LogicalAggregate.NONE.[](input=RelSubset#,group={},cnt=COUNT($0)), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":0,"timeout":"1m","query":{"exists":{"field":"lastname","boost":1.0}},"track_total_hits":2147483647}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)]) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java new file mode 100644 index 00000000000..990170f7a34 --- /dev/null +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java @@ -0,0 +1,76 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl.calcite; + +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.test.CalciteAssert; +import org.junit.Test; + +/** + * {@code stats count(field)} adds an is-not-null filter, and these pin the frame its reference is + * expressed in. + * + *

The filter is stacked on whatever the builder is holding, so the reference has to address that + * node's output. A {@code fields} command in front of the {@code stats} makes the difference + * visible: the column's index in the projection's output is not its index in the projection's + * input, and using the latter is out of range as soon as the projection is narrower than its input. + */ +public class CalcitePPLCountFrameTest extends CalcitePPLAbstractTest { + + public CalcitePPLCountFrameTest() { + super(CalciteAssert.SchemaSpec.SCOTT_WITH_TEMPORAL); + } + + /** COMM is $6 of the scan but $0 of the projection the filter sits on. */ + @Test + public void testCountAfterNarrowingFields() { + String ppl = "source=EMP | fields COMM | stats count(COMM) as c"; + RelNode root = getRelNode(ppl); + String expectedLogical = + "" + + "LogicalAggregate(group=[{}], c=[COUNT($0)])\n" + + " LogicalFilter(condition=[IS NOT NULL($0)])\n" + + " LogicalProject(COMM=[$6])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + verifyResult(root, "c=4\n"); + } + + /** Two columns kept, so the index is in range either way but only one of them is COMM. */ + @Test + public void testCountAfterReorderingFields() { + String ppl = "source=EMP | fields SAL, COMM | stats count(COMM) as c"; + RelNode root = getRelNode(ppl); + String expectedLogical = + "" + + "LogicalAggregate(group=[{}], c=[COUNT($0)])\n" + + " LogicalProject(COMM=[$1])\n" + + " LogicalFilter(condition=[IS NOT NULL($1)])\n" + + " LogicalProject(SAL=[$5], COMM=[$6])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + verifyLogical(root, expectedLogical); + verifyResult(root, "c=4\n"); + } + + /** dc() takes the same filter, so it takes the same reference. */ + @Test + public void testDistinctCountAfterNarrowingFields() { + String ppl = "source=EMP | fields COMM | stats dc(COMM) as c"; + RelNode root = getRelNode(ppl); + verifyResult(root, "c=4\n"); + } + + /** + * Two names for one column, which is the case the index mapping exists for: it recognises them as + * the same column so a single filter covers both counts. + */ + @Test + public void testCountOfAnAliasedColumn() { + String ppl = "source=EMP | eval bonus = COMM | fields bonus | stats count(bonus) as c"; + RelNode root = getRelNode(ppl); + verifyResult(root, "c=4\n"); + } +} From ea8f6644bd1aa7babcb89c1a251e7aec86b880cf Mon Sep 17 00:00:00 2001 From: Marc Handalian Date: Mon, 21 Sep 2026 20:31:03 +0000 Subject: [PATCH 2/2] Test the wrong-column case, and trim the comments The four tests only proved the out-of-range crash was gone, and the reordering one was a duplicate of the narrowing one: `fields SAL, COMM` is two columns wide, so $6 still overflows. Replace it with the case that was missing. `fields EMPNO, ENAME, JOB, MGR, HIREDATE, COMM, SAL` puts SAL at output position 6, so the input-side $6 stays in range and resolves to SAL while COMM is $5, and both are DECIMAL. The count is still 4 because count(field) ignores nulls on its own, so it asserts the plan rather than the result. Signed-off-by: Marc Handalian --- .../sql/calcite/CalciteRelNodeVisitor.java | 13 ++----- .../ppl/calcite/CalcitePPLCountFrameTest.java | 37 ++++++++++--------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index 003211d60d2..e66e3319b57 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -1616,15 +1616,10 @@ private Pair, List> aggregateWithTrimming( distinctRefsOfCounts = refsPerCount.stream().flatMap(List::stream).distinct().toList(); } if (distinctRefsOfCounts.size() == 1 && refsPerCount.stream().noneMatch(List::isEmpty)) { - // This filter is stacked on top of the Project, so its reference has to address the - // Project's OUTPUT. distinctRefsOfCounts may instead hold an index mapped through the - // Project, which addresses its INPUT — that mapping exists only to prove two aliases are - // one column, and it is not a reference we can use here. `fields COMM | stats count(COMM)` - // is the clearest case: COMM is $6 of the scan and $0 of the projection, and filtering on - // $6 fails with "RexInputRef index 6 out of range 0..0". Where the projection merely - // reorders, the index stays in range and names a different column, so the wrong column's - // nullness is tested. refsPerCount is already in the output frame, and by this branch every - // entry denotes the same column, so any one of them gives the same predicate. + // The filter is stacked on the Project, so its reference must address the Project's output. + // distinctRefsOfCounts may hold an index mapped through the Project, which addresses its + // input; that mapping only serves to prove two aliases are one column. refsPerCount is + // already in the output frame, and every entry here denotes the same column. RexInputRef filterRef = refsPerCount.stream().flatMap(List::stream).findFirst().orElseThrow(); context.relBuilder.filter(context.relBuilder.isNotNull(filterRef)); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java index 990170f7a34..90621da5bbf 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLCountFrameTest.java @@ -10,13 +10,11 @@ import org.junit.Test; /** - * {@code stats count(field)} adds an is-not-null filter, and these pin the frame its reference is - * expressed in. + * The is-not-null filter that {@code stats count(field)} adds sits on the projection the aggregate + * reads, so its reference must be the column's index in that projection's output. An input-side + * index is out of range when the projection narrows, and names the wrong column when it does not. * - *

The filter is stacked on whatever the builder is holding, so the reference has to address that - * node's output. A {@code fields} command in front of the {@code stats} makes the difference - * visible: the column's index in the projection's output is not its index in the projection's - * input, and using the latter is out of range as soon as the projection is narrower than its input. + *

EMP is {@code EMPNO $0, ENAME $1, JOB $2, MGR $3, HIREDATE $4, SAL $5, COMM $6, DEPTNO $7}. */ public class CalcitePPLCountFrameTest extends CalcitePPLAbstractTest { @@ -24,7 +22,7 @@ public CalcitePPLCountFrameTest() { super(CalciteAssert.SchemaSpec.SCOTT_WITH_TEMPORAL); } - /** COMM is $6 of the scan but $0 of the projection the filter sits on. */ + /** COMM is $6 of the scan and $0 of the projection, so an input-side index is out of range. */ @Test public void testCountAfterNarrowingFields() { String ppl = "source=EMP | fields COMM | stats count(COMM) as c"; @@ -39,23 +37,29 @@ public void testCountAfterNarrowingFields() { verifyResult(root, "c=4\n"); } - /** Two columns kept, so the index is in range either way but only one of them is COMM. */ + /** + * SAL sits at output position 6, so $6 stays in range and resolves to SAL while COMM is $5. Both + * are DECIMAL, so nothing objects. The result stays 4 because count ignores nulls itself, so only + * the plan shows which column the filter is about. + */ @Test - public void testCountAfterReorderingFields() { - String ppl = "source=EMP | fields SAL, COMM | stats count(COMM) as c"; + public void testCountWhenTheStaleIndexStaysInRange() { + String ppl = + "source=EMP | fields EMPNO, ENAME, JOB, MGR, HIREDATE, COMM, SAL | stats count(COMM) as c"; RelNode root = getRelNode(ppl); String expectedLogical = "" + "LogicalAggregate(group=[{}], c=[COUNT($0)])\n" - + " LogicalProject(COMM=[$1])\n" - + " LogicalFilter(condition=[IS NOT NULL($1)])\n" - + " LogicalProject(SAL=[$5], COMM=[$6])\n" + + " LogicalProject(COMM=[$5])\n" + + " LogicalFilter(condition=[IS NOT NULL($5)])\n" + + " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4]," + + " COMM=[$6], SAL=[$5])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); verifyResult(root, "c=4\n"); } - /** dc() takes the same filter, so it takes the same reference. */ + /** dc() takes the same filter, so it took the same out-of-range reference. */ @Test public void testDistinctCountAfterNarrowingFields() { String ppl = "source=EMP | fields COMM | stats dc(COMM) as c"; @@ -63,10 +67,7 @@ public void testDistinctCountAfterNarrowingFields() { verifyResult(root, "c=4\n"); } - /** - * Two names for one column, which is the case the index mapping exists for: it recognises them as - * the same column so a single filter covers both counts. - */ + /** Two names for one column, which is what the input-side mapping is kept for. */ @Test public void testCountOfAnAliasedColumn() { String ppl = "source=EMP | eval bonus = COMM | fields bonus | stats count(bonus) as c";