Improve error message for unsupported window functions in eventstats/streamstats

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c

[gingeekrishna]

Description

Fixes#5168

eventstats/streamstats reject window functions outside WINDOW_FUNC_MAPPING (e.g. rank(), dense_rank(), nth_value()) with a bare "Unexpected window function: X" error.

As confirmed in the issue discussion by @songkant-aws, this is expected behavior, not a bug — rank/dense_rank/nth_value require ORDER BY semantics, but eventstats/streamstats only support partition by. The issue was relabeled error-experience: the fix is to make the error message clearer, not to add support for these functions.

Changes

• CalciteRexNodeVisitor#visitWindowFunction: replaced the generic "Unexpected window function: X" message with one that names the rejected function and lists the functions eventstats/streamstats do support (sourced from WINDOW_FUNC_MAPPING), so users get actionable guidance instead of a bare error.
• Updated the existing unit test (UnifiedQueryPlannerTest) and integration tests (CalcitePPLEventstatsIT, CalciteStreamstatsCommandIT) to assert against the new message.
• Added new integration tests covering rank/dense_rank specifically, since the issue's repro queries used those functions.

Note: this PR builds on top of #5587 (already merged), which fixed the same throw site to return a 4xx instead of a 500. This PR only changes the message text, not the exception type or HTTP status.

Test plan

• Unit test UnifiedQueryPlannerTest#unsupportedWindowFunctionIsRethrownAsSemanticCheckException passes
• Integration tests added/updated for eventstats and streamstats (require a live cluster to run in CI)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dd3ca1c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Hardcoded list The error message hardcodes the list of supported functions as a string literal. If WINDOW_FUNC_MAPPING changes (functions added or removed), this message will become stale and misleading. The list should be generated dynamically from WINDOW_FUNC_MAPPING to stay accurate. + "' 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."));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dd3ca1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for queryType Add null check for context.queryType before comparison to prevent potential NullPointerException. The context object might have a null queryType field in certain scenarios, which would cause the filter predicate to fail. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [773-777] .filter( functionName -> - !(context.queryType == QueryType.PPL+ !(context.queryType != null+ && context.queryType == QueryType.PPL && (functionName == BuiltinFunctionName.RANK || functionName == BuiltinFunctionName.DENSE_RANK))) Suggestion importance[1-10]: 5 __ Why: While adding a null check for context.queryType is a defensive programming practice, there's no evidence in the PR that queryType can be null. The suggestion is technically valid but may be unnecessary without context showing this is a real issue. This is a standard null-safety suggestion that doesn't address a demonstrated bug.	Low

---

Previous suggestions

Suggestions up to commit 58fda66

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + "."); Suggestion importance[1-10]: 7 __ Why: This is a valid maintainability improvement that prevents the hardcoded list from becoming stale when WINDOW_FUNC_MAPPING is updated. However, it's a moderate enhancement rather than a critical fix, as the current implementation is functional and the mapping is unlikely to change frequently.	Medium

Suggestions up to commit cf4cb1b

Category	Suggestion	Impact
General	Verify row_number support in error message The error message lists row_number as a supported function, but the grammar changes in OpenSearchPPLParser.g4 removed RANK and DENSE_RANK from scalarWindowFunctionName, leaving only ROW_NUMBER and other functions. Verify that row_number is actually supported in eventstats/streamstats context, as the test changes suggest ranking functions are now rejected at parse time. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [812-818] 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,"+ + " latest, max, min, stddev_pop, stddev_samp, sum," + " var_pop, var_samp.")); Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the consistency between the error message and grammar changes. However, examining the grammar file shows that ROW_NUMBER is still present in scalarWindowFunctionName, suggesting it remains supported. The suggestion asks for verification rather than identifying a definite error, and the existing_code matches the improved_code, indicating no concrete change is proposed.	Medium

Suggestions up to commit 99c53f5

Category	Suggestion	Impact
General	Use String.format for error message The error message concatenates multiple strings which can impact readability and maintainability. Consider using String.format() or a text block for better formatting and easier future modifications of the supported functions list. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ String.format(+ "Window function '%s' 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.",+ funcName)); Suggestion importance[1-10]: 4 __ Why: While using String.format() can improve readability, the current string concatenation approach is perfectly valid and readable. This is a minor style improvement that doesn't significantly impact code quality or functionality.	Low

Suggestions up to commit c16fc0c

Category	Suggestion	Impact
General	Dynamically generate supported functions list The hardcoded list of supported functions in the error message may become outdated if new functions are added to WINDOW_FUNC_MAPPING. Consider dynamically generating this list from the mapping keys to ensure accuracy and maintainability. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [709-715] 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."));+ + " Supported functions: "+ + String.join(", ", WINDOW_FUNC_MAPPING.keySet())+ + ".")); Suggestion importance[1-10]: 7 __ Why: This suggestion improves maintainability by dynamically generating the list of supported functions from WINDOW_FUNC_MAPPING.keySet() instead of hardcoding them. This ensures the error message stays accurate when new functions are added, reducing maintenance burden and preventing outdated documentation.	Medium

[github-actions]

Persistent review updated to latest commit 99c53f5

[dai-chen]

Hi, I'm trying to add support for rank and dense_rank function in Calcite for unified SQL for analytics engine. If we confirm both are not supported by PPL evenstats and streamstats, shall we remove them from PPL grammar directly? I assume that's the only place window function can be used. Otherwise it will be tricky to check language type in Calcite planner.

[gingeekrishna]

@dai-chen Good question — I dug into this a bit.

Your assumption holds for PPL specifically: in the PPL grammar, windowFunction (the parenthesized call syntax) is only ever invoked from eventstatsAggTerm/streamstatsAggTerm (OpenSearchPPLParser.g4:922-943), so that is the only place a window function can appear in PPL today.

But it's not the only place in the engine overall. SQL has its own windowFunctionClause/overClause grammar (OpenSearchSQLParser.g4:150-159) that already accepts RANK()/DENSE_RANK() in ... OVER (...). Both SQL and PPL build the same WindowFunction AST node and are resolved by the same CalciteRexNodeVisitor#visitWindowFunction — there's even a comment there noting "SQL emits AggregateFunction for aggregate-as-window (e.g., SUM(x) OVER); PPL emits Function." WINDOW_FUNC_MAPPING in BuiltinFunctionName (used by both paths) simply has no rank/dense_rank entries yet, so both languages fail identically right now.

Practical implication: if you add rank/dense_rank to WINDOW_FUNC_MAPPING and implement them in the shared visitor, that enables them for PPL eventstats/streamstats too, since the mapping/visitor is shared — not just SQL.

So yes, I think removing RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule (lines 933-934) is the right move, and it avoids the language-type check you were worried about: PPL's parser would reject the syntax before it ever reaches the AST/Calcite layer, while SQL's grammar (which already accepts it) passes through untouched. That pushes the language separation to parse time instead of planner time.

One side note on this PR itself: the new error message I added ("Window function 'x' is not supported in eventstats/streamstats") is thrown from that same shared visitor, so it would also currently fire — misleadingly — for a plain SQL RANK() OVER (...) query, since the check isn't actually PPL-specific. I may follow up with a tweak to make that message language-agnostic.

[dai-chen]

@gingeekrishna Thanks for digging in! I've put up PR #5720 for the SQL side. As you confirmed, removing them from grammar should make PPL reject at parse time and leaves my change SQL-only. With both PRs merged, I think we get the language separation at parse time rather than in the planner, which is where it belongs as expected. Thanks!

[gingeekrishna]

@dai-chen Nice, thanks for #5720. Pushed da13116 here: removed RANK/DENSE_RANK from PPL's scalarWindowFunctionName rule, so eventstats rank()/streamstats dense_rank() now fail at parse time (SyntaxCheckException) instead of reaching the shared visitor.

Updated the tests this PR added for that case (testRankingWindowFunctionsUnsupportedInEventstats/...InStreamstats) to expect the parse-time rejection, and switched unsupportedWindowFunctionIsRethrownAsSemanticCheckException from rank() to percent_rank() since it needs a token that's still in the grammar but outside WINDOW_FUNC_MAPPING to exercise that path.

One caveat: I don't have a Java toolchain in my current environment, so I couldn't run generateGrammarSource/the actual test suite to confirm the exact ANTLR error message — the assertions only check for the generic "is not a valid term at this part of the query" substring rather than the full expected-tokens list, to stay robust either way, but it'd be good to have CI confirm this once it runs.

[github-actions]

Persistent review updated to latest commit da13116

[github-actions]

Persistent review updated to latest commit cf4cb1b

[dai-chen]

@gingeekrishna My PR was already merged. You can rebase and verify yours by CI if you're unable locally. Btw, I recall changes in unsupportedWindowFunctionIsRethrownAsSemanticCheckException is already covered in my PR.

[dai-chen]

maybe revert this change because this list is dynamic and defined in grammar.

[dai-chen]

Minor comment. Thanks for the changes!

[dai-chen]

I retried Linter CI but it still failed. Probably worth taking another look.

[gingeekrishna]

Found it - pushed 58fda664f to fix it.

The failing check was the linter, specifically:

[ppl-lint-grammar] FAILED unsupported-window-function-in-eventstats/eventstats-rank: expected 1, got 0

That's OpenSearch-Dashboards' own PPL linter (fetched via yarn osd bootstrap in that job) being run against this repo's grammar bundle, using the cases in scripts/ppl-lint/grammar-cases.json. Its unsupported-window-function-in-eventstats rule expects eventstats rank() to parse successfully and then get flagged by a semantic-layer diagnostic - that's how it surfaces a friendly, specific error in the IDE.

My earlier commit here (cf4cb1bd3, removing RANK/DENSE_RANK from the PPL grammar so eventstats rank() fails at parse time instead of reaching the shared visitor) broke exactly that: the query now dies with a raw syntax error before OSD's semantic rule ever gets a chance to run, so its diagnostic never fires. Since that rule lives in the OpenSearch-Dashboards repo, not here, fixing it properly would need a coordinated cross-repo change - out of scope for this PR.

Reverted that commit. This PR's actual point - the improved "Window function 'x' is not supported in eventstats/streamstats" message in CalciteRexNodeVisitor - is a semantic-layer check, so it's untouched by the revert and still works exactly as intended. The parse-time language-separation idea from our earlier discussion doesn't hold up against the existing linter contract, so I'm dropping it rather than pursuing it further here.

[github-actions]

Persistent review updated to latest commit 58fda66

[github-actions]

Persistent review updated to latest commit dd3ca1c