From 7bae4996b845bc129950dd806695e9bd92606fa0 Mon Sep 17 00:00:00 2001 From: Radhakrishnan Pachyappan Date: Tue, 30 Jun 2026 21:20:44 +0530 Subject: [PATCH 1/4] Improve error message for unsupported window functions in eventstats/streamstats Window functions outside WINDOW_FUNC_MAPPING (e.g. rank, dense_rank, nth_value) throw a generic "Unexpected window function: X" from CalciteRexNodeVisitor#visitWindowFunction. These functions require ORDER BY semantics that eventstats/streamstats don't have (they only support partition-by), so they are intentionally unsupported, not a bug -- but the error message gave users no indication of what to use instead. Replace the message with one that names the function and lists the functions eventstats/streamstats do support, so users get actionable guidance instead of a bare "unexpected" error. Fixes #5168 Signed-off-by: Radhakrishnan Pachyappan --- .../sql/api/UnifiedQueryPlannerTest.java | 3 ++- .../sql/calcite/CalciteRexNodeVisitor.java | 9 ++++++++- .../calcite/remote/CalcitePPLEventstatsIT.java | 17 ++++++++++++++++- .../remote/CalciteStreamstatsCommandIT.java | 18 +++++++++++++++++- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java index f8cc834dee8..00bceba9fe2 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java @@ -171,7 +171,8 @@ public void unsupportedWindowFunctionIsRethrownAsSemanticCheckException() { givenInvalidQuery("source = catalog.employees | eventstats percent_rank()") .assertErrorType(SemanticCheckException.class) .assertCauseType(CalciteUnsupportedException.class) - .assertErrorMessageContains("Unexpected window function: percent_rank"); + .assertErrorMessageContains( + "Window function 'percent_rank' is not supported in eventstats/streamstats"); } @Test diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index b6500dd8087..b554cb82227 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -808,7 +808,14 @@ public RexNode visitWindowFunction(WindowFunction node, CalcitePlanContext conte node.getWindowFrame()); }) .orElseThrow( - () -> new CalciteUnsupportedException("Unexpected window function: " + funcName)); + () -> + new CalciteUnsupportedException( + "Window function '" + + funcName + + "' is not supported in eventstats/streamstats." + + " Supported functions: avg, count, dc, distinct_count, earliest," + + " latest, max, min, row_number, stddev_pop, stddev_samp, sum," + + " var_pop, var_samp.")); } private List translateOrderKeys( diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java index a2d1e4af086..36f870f4a4e 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java @@ -329,7 +329,22 @@ public void testUnsupportedWindowFunctions() { executeQuery( String.format( "source=%s | eventstats %s(age)", TEST_INDEX_STATE_COUNTRY, u))); - verifyErrorMessageContains(e, "Unexpected window function: " + u); + verifyErrorMessageContains(e, "is not supported in eventstats/streamstats"); + } + } + + @Test + public void testRankingWindowFunctionsUnsupportedInEventstats() { + for (String func : List.of("rank", "dense_rank")) { + Throwable e = + assertThrowsWithReplace( + UnsupportedOperationException.class, + () -> + executeQuery( + String.format( + "source=%s | eventstats %s() by state", TEST_INDEX_STATE_COUNTRY, func))); + verifyErrorMessageContains( + e, "Window function '" + func + "' is not supported in eventstats/streamstats"); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java index e70812e3c3b..26407b08dfc 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java @@ -846,7 +846,23 @@ public void testUnsupportedWindowFunctions() { executeQuery( String.format( "source=%s | streamstats %s(age)", TEST_INDEX_STATE_COUNTRY, u))); - verifyErrorMessageContains(e, "Unexpected window function: " + u); + verifyErrorMessageContains(e, "is not supported in eventstats/streamstats"); + } + } + + @Test + public void testRankingWindowFunctionsUnsupportedInStreamstats() { + for (String func : List.of("rank", "dense_rank")) { + Throwable e = + assertThrowsWithReplace( + UnsupportedOperationException.class, + () -> + executeQuery( + String.format( + "source=%s | streamstats %s() by state", + TEST_INDEX_STATE_COUNTRY, func))); + verifyErrorMessageContains( + e, "Window function '" + func + "' is not supported in eventstats/streamstats"); } } From cf4cb1bd347eeb1dbf06f6bfbcf9cba8928953f5 Mon Sep 17 00:00:00 2001 From: Radhakrishnan P Date: Sat, 29 Aug 2026 00:33:48 +0530 Subject: [PATCH 2/4] Reject rank/dense_rank in PPL eventstats/streamstats at parse time WINDOW_FUNC_MAPPING (used by eventstats/streamstats) never supported rank/dense_rank, but PPL's scalarWindowFunctionName grammar rule still accepted the tokens, so `eventstats rank()` reached CalciteRexNodeVisitor#visitWindowFunction and failed there with the "not supported" message this PR improves. Meanwhile SQL's grammar already accepts RANK()/DENSE_RANK() OVER (...), and #5720 is adding real support for them on the SQL side via the same shared visitor. Remove RANK/DENSE_RANK from scalarWindowFunctionName so PPL rejects them at parse time instead of falling through to the shared SQL/PPL visitor - this keeps the language separation at the parser rather than relying on a WINDOW_FUNC_MAPPING check in shared planner code, and avoids PPL silently gaining rank/dense_rank as a side effect of #5720 registering them for SQL. Updates the eventstats/streamstats tests added earlier in this PR to expect a SyntaxCheckException (parse-time) instead of the semantic "not supported" error, and switches the unrelated visitWindowFunction-rejection unit test from rank() to percent_rank(), which remains unsupported and still exercises that code path. Signed-off-by: Radhakrishnan P --- .../sql/calcite/remote/CalcitePPLEventstatsIT.java | 8 +++++--- .../sql/calcite/remote/CalciteStreamstatsCommandIT.java | 8 +++++--- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 2 -- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java index 36f870f4a4e..9c0bd5422c5 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java @@ -12,6 +12,7 @@ import java.util.List; import org.json.JSONObject; import org.junit.jupiter.api.Test; +import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.ppl.PPLIntegTestCase; public class CalcitePPLEventstatsIT extends PPLIntegTestCase { @@ -335,16 +336,17 @@ public void testUnsupportedWindowFunctions() { @Test public void testRankingWindowFunctionsUnsupportedInEventstats() { + // rank/dense_rank aren't in eventstats/streamstats' windowFunctionName grammar rule, so this + // is now a parse-time rejection rather than a semantic one. for (String func : List.of("rank", "dense_rank")) { Throwable e = assertThrowsWithReplace( - UnsupportedOperationException.class, + SyntaxCheckException.class, () -> executeQuery( String.format( "source=%s | eventstats %s() by state", TEST_INDEX_STATE_COUNTRY, func))); - verifyErrorMessageContains( - e, "Window function '" + func + "' is not supported in eventstats/streamstats"); + verifyErrorMessageContains(e, "is not a valid term at this part of the query"); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java index 26407b08dfc..81c653b0742 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java @@ -16,6 +16,7 @@ import org.json.JSONObject; import org.junit.jupiter.api.Test; import org.opensearch.client.Request; +import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.ppl.PPLIntegTestCase; import org.opensearch.sql.util.RequiresCapability; @@ -852,17 +853,18 @@ public void testUnsupportedWindowFunctions() { @Test public void testRankingWindowFunctionsUnsupportedInStreamstats() { + // rank/dense_rank aren't in eventstats/streamstats' windowFunctionName grammar rule, so this + // is now a parse-time rejection rather than a semantic one. for (String func : List.of("rank", "dense_rank")) { Throwable e = assertThrowsWithReplace( - UnsupportedOperationException.class, + SyntaxCheckException.class, () -> executeQuery( String.format( "source=%s | streamstats %s() by state", TEST_INDEX_STATE_COUNTRY, func))); - verifyErrorMessageContains( - e, "Window function '" + func + "' is not supported in eventstats/streamstats"); + verifyErrorMessageContains(e, "is not a valid term at this part of the query"); } } diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index ce949f8534b..b8280043aeb 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -930,8 +930,6 @@ windowFunctionName scalarWindowFunctionName : ROW_NUMBER - | RANK - | DENSE_RANK | PERCENT_RANK | CUME_DIST | FIRST From 58fda664fe07ad244cce66bf3afdff0b1d2f99ab Mon Sep 17 00:00:00 2001 From: Radhakrishnan P Date: Wed, 2 Sep 2026 09:39:59 +0530 Subject: [PATCH 3/4] Revert "Reject rank/dense_rank in PPL eventstats/streamstats at parse time" This reverts commit cf4cb1bd347eeb1dbf06f6bfbcf9cba8928953f5. The "PPL grammar compatibility" CI check failed after that commit: [ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/ eventstats-rank: expected 1, got 0 That check runs OpenSearch-Dashboards' own PPL linter (fetched via `yarn osd bootstrap`) against this repo's grammar bundle, using test cases from scripts/ppl-lint/grammar-cases.json. Its "unsupported-window-function-in-eventstats" rule expects `eventstats rank()` to parse successfully and then be flagged by a semantic-layer diagnostic (giving IDE users a specific, friendly error). Removing RANK/DENSE_RANK from the PPL grammar made the query fail to parse at all, so that rule never gets a chance to run and the diagnostic it's supposed to produce disappears - the ppl-lint test expects the rule to fire (count 1) but the query now dies earlier with a raw syntax error instead (count 0). The rule implementation lives in OpenSearch-Dashboards, not this repo, so fixing this properly would need a coordinated cross-repo change. Reverting restores parsing (and the linter's diagnostic) while keeping the actual point of this PR - the improved "Window function 'x' is not supported in eventstats/streamstats" message from CalciteRexNodeVisitor - fully intact, since that's a semantic-layer check unaffected by this revert. Signed-off-by: Radhakrishnan P --- .../sql/calcite/remote/CalcitePPLEventstatsIT.java | 8 +++----- .../sql/calcite/remote/CalciteStreamstatsCommandIT.java | 8 +++----- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 2 ++ 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java index 9c0bd5422c5..36f870f4a4e 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEventstatsIT.java @@ -12,7 +12,6 @@ import java.util.List; import org.json.JSONObject; import org.junit.jupiter.api.Test; -import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.ppl.PPLIntegTestCase; public class CalcitePPLEventstatsIT extends PPLIntegTestCase { @@ -336,17 +335,16 @@ public void testUnsupportedWindowFunctions() { @Test public void testRankingWindowFunctionsUnsupportedInEventstats() { - // rank/dense_rank aren't in eventstats/streamstats' windowFunctionName grammar rule, so this - // is now a parse-time rejection rather than a semantic one. for (String func : List.of("rank", "dense_rank")) { Throwable e = assertThrowsWithReplace( - SyntaxCheckException.class, + UnsupportedOperationException.class, () -> executeQuery( String.format( "source=%s | eventstats %s() by state", TEST_INDEX_STATE_COUNTRY, func))); - verifyErrorMessageContains(e, "is not a valid term at this part of the query"); + verifyErrorMessageContains( + e, "Window function '" + func + "' is not supported in eventstats/streamstats"); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java index 81c653b0742..26407b08dfc 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java @@ -16,7 +16,6 @@ import org.json.JSONObject; import org.junit.jupiter.api.Test; import org.opensearch.client.Request; -import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.ppl.PPLIntegTestCase; import org.opensearch.sql.util.RequiresCapability; @@ -853,18 +852,17 @@ public void testUnsupportedWindowFunctions() { @Test public void testRankingWindowFunctionsUnsupportedInStreamstats() { - // rank/dense_rank aren't in eventstats/streamstats' windowFunctionName grammar rule, so this - // is now a parse-time rejection rather than a semantic one. for (String func : List.of("rank", "dense_rank")) { Throwable e = assertThrowsWithReplace( - SyntaxCheckException.class, + UnsupportedOperationException.class, () -> executeQuery( String.format( "source=%s | streamstats %s() by state", TEST_INDEX_STATE_COUNTRY, func))); - verifyErrorMessageContains(e, "is not a valid term at this part of the query"); + verifyErrorMessageContains( + e, "Window function '" + func + "' is not supported in eventstats/streamstats"); } } diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index b8280043aeb..ce949f8534b 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -930,6 +930,8 @@ windowFunctionName scalarWindowFunctionName : ROW_NUMBER + | RANK + | DENSE_RANK | PERCENT_RANK | CUME_DIST | FIRST From dd3ca1c29aea49164e5b33ff7be854e32e2c0c99 Mon Sep 17 00:00:00 2001 From: Radhakrishnan P Date: Wed, 2 Sep 2026 10:29:35 +0530 Subject: [PATCH 4/4] Reject rank/dense_rank in PPL eventstats/streamstats at the semantic layer #5720 added rank/dense_rank to the WINDOW_FUNC_MAPPING shared by both SQL's RANK()/DENSE_RANK() OVER (...) and PPL's eventstats/streamstats, which silently enabled them for eventstats/streamstats too: testRankingWindowFunctionsUnsupportedInEventstats/InStreamstats (added earlier in this PR) started failing with "expected ResponseException to be thrown, but nothing was thrown" once #5720 merged, since eventstats rank() now builds a real window call instead of hitting the "not supported" check. That's a real gap, not just a test artifact: PPL's eventstats/ streamstats grammar has no ORDER BY syntax at all, so ranking has no defined ordering to rank by there - unlike SQL's OVER(), which at least has (optional) ORDER BY in its own clause. A prior commit on this branch tried fixing this by removing RANK/ DENSE_RANK from the PPL grammar entirely, rejecting them at parse time. That broke a different, cross-repo contract: OpenSearch- Dashboards' PPL linter (validated by the "PPL grammar compatibility" CI check) expects `eventstats rank()` to parse successfully and be flagged by a semantic-layer diagnostic instead, so it could no longer produce that diagnostic once the query stopped parsing. That commit was reverted. Fix this at the semantic layer instead, where it belongs: PPL only ever reaches CalciteRexNodeVisitor#visitWindowFunction through eventstats/streamstats (no other PPL syntax builds a WindowFunction node), so context.queryType == PPL is an exact, unambiguous signal for "this is an eventstats/streamstats call". Filter rank/dense_rank out of the WINDOW_FUNC_MAPPING lookup specifically when queryType is PPL, so they fall through to the existing "not supported in eventstats/ streamstats" error - exactly the pre-#5720 behavior - while leaving SQL's RANK()/DENSE_RANK() OVER (...) handling (and everything else) completely untouched. Signed-off-by: Radhakrishnan P --- .../sql/calcite/CalciteRexNodeVisitor.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index b554cb82227..c0944616a01 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -95,6 +95,7 @@ import org.opensearch.sql.exception.CalciteUnsupportedException; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.executor.QueryType; import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.expression.function.CoercionUtils; import org.opensearch.sql.expression.function.PPLBuiltinOperators; @@ -763,6 +764,17 @@ public RexNode visitWindowFunction(WindowFunction node, CalcitePlanContext conte .toList(); List orderKeys = translateOrderKeys(node.getSortList(), context); return BuiltinFunctionName.ofWindowFunction(funcName) + // rank/dense_rank were added to WINDOW_FUNC_MAPPING to support SQL's RANK()/DENSE_RANK() + // OVER (...), but PPL only ever reaches this visitor through eventstats/streamstats + // (there's no other PPL syntax that builds a WindowFunction node), which don't support + // them - PPL has no ORDER BY syntax for eventstats/streamstats, so ranking would be + // meaningless there. Excluding them here for PPL keeps that restriction despite the + // shared mapping, without needing a PPL-specific grammar change. + .filter( + functionName -> + !(context.queryType == QueryType.PPL + && (functionName == BuiltinFunctionName.RANK + || functionName == BuiltinFunctionName.DENSE_RANK))) .map( functionName -> { RexNode field = arguments.isEmpty() ? null : arguments.getFirst();