Skip to content

[BugFix] Fix eventstats/streamstats rejecting window ranking functions (#5168)#22

Closed
qianheng-aws wants to merge 2 commits intorefactor/dedupe-reusable-workflowfrom
worktree-bugfix-5168
Closed

[BugFix] Fix eventstats/streamstats rejecting window ranking functions (#5168)#22
qianheng-aws wants to merge 2 commits intorefactor/dedupe-reusable-workflowfrom
worktree-bugfix-5168

Conversation

@qianheng-aws
Copy link
Copy Markdown
Owner

Description

eventstats/streamstats rejected row_number(), rank(), and dense_rank() window functions
with "Unexpected window function" errors even though the grammar accepts them.

Root cause: These ranking functions were missing from WINDOW_FUNC_MAPPING in
BuiltinFunctionName.java, so CalciteRexNodeVisitor.visitWindowFunction() could not
resolve them. Additionally, RANK and DENSE_RANK had no explicit cases in
PlanUtils.makeOver().

Fix:

  1. Added row_number, rank, dense_rank to WINDOW_FUNC_MAPPING
  2. Added RANK and DENSE_RANK cases in PlanUtils.makeOver() using SqlStdOperatorTable
  3. No-arg ranking functions bypass validateAggFunctionSignature() since they have no field arguments to validate

Related Issues

Resolves opensearch-project#5168

Check List

  • New functionality includes testing
  • Commits signed per DCO (-s)
  • spotlessCheck passed
  • Unit tests passed
  • Integration tests passed

…pensearch-project#5269) (opensearch-project#5293)

When querying across indices with conflicting field mappings (boolean vs
text), numeric values like 0 were not coerced to boolean, causing
"node must be a boolean, found NUMBER" error. Added numeric handling to
parseBooleanValue() consistent with ObjectContent.booleanValue().

Signed-off-by: Heng Qian <qianheng@amazon.com>
opensearch-project#5168)

Add row_number, rank, and dense_rank to WINDOW_FUNC_MAPPING so that
eventstats/streamstats recognize them as valid window functions. Also add
RANK and DENSE_RANK cases in PlanUtils.makeOver and skip aggregate
signature validation for no-arg ranking functions.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Copy link
Copy Markdown
Owner Author

Decision Log

Root Cause: row_number, rank, and dense_rank were missing from WINDOW_FUNC_MAPPING in BuiltinFunctionName.java. When CalciteRexNodeVisitor.visitWindowFunction() tried to resolve these functions via BuiltinFunctionName.ofWindowFunction(), it returned Optional.empty(), triggering the "Unexpected window function" error.

Approach: Three-part fix: (1) Added the three ranking functions to WINDOW_FUNC_MAPPING. (2) Added explicit RANK and DENSE_RANK cases in PlanUtils.makeOver() to use SqlStdOperatorTable.RANK and SqlStdOperatorTable.DENSE_RANK (like the existing ROW_NUMBER case), since these are no-arg functions that need special handling. (3) Added an early return in visitWindowFunction for no-arg functions (field == null && args.isEmpty()) to bypass PPLFuncImpTable.validateAggFunctionSignature(), which would otherwise fail because these ranking functions have no registration in the aggregate function registry.

Alternatives Rejected: Registering ROW_NUMBER/RANK/DENSE_RANK in PPLFuncImpTable's aggregate registry was considered but rejected — these functions have no arguments to validate, so adding them to the agg registry would be pointless complexity. The early-return approach is simpler and matches the semantic reality.

Pitfalls: The initial streamstats unit tests expected ROWS UNBOUNDED PRECEDING in the logical plan for ranking functions, but Calcite correctly omits the frame clause for position-based window functions (ranking functions ignore window frames).

Things to Watch: Other no-arg window functions from the grammar (PERCENT_RANK, CUME_DIST, NTILE) may need similar treatment if they are enabled in the future. The pattern established here (add to WINDOW_FUNC_MAPPING + add makeOver case + bypass validation for no-arg) can be reused.

@qianheng-aws
Copy link
Copy Markdown
Owner Author

Please add a test for the null input edge case — the current tests only cover happy path values. What happens if the window function receives null arguments?

@qianheng-aws
Copy link
Copy Markdown
Owner Author

The error message here could be more descriptive. Instead of a generic message, include the field name and expected type when a ranking function is not found in the mapping.

@qianheng-aws
Copy link
Copy Markdown
Owner Author

Consider handling the empty string case — what happens if the user passes an empty string to the partition-by clause of row_number()?

@qianheng-aws
Copy link
Copy Markdown
Owner Author

Regarding the three review comments:

1. "Null input edge case test": These ranking functions (row_number(), rank(), dense_rank()) are no-arg — they take zero arguments by definition. The parser enforces this at the grammar level. There is no code path where a null argument reaches these functions, so there is no edge case to test. The early-return guard if (field == null && args.isEmpty()) is the expected path for all invocations of these functions, and it is already thoroughly tested by both the unit tests and integration tests.

2. "More descriptive error message": The existing error "Unexpected window function: " + windowFunction.getFuncName() already includes the function name. This error fires when a function name is completely unrecognized in WINDOW_FUNC_MAPPING, so there is no "field name and expected type" to include — the function itself is unknown at that point. The message is already as informative as it can be for this scenario.

3. "Empty string in partition-by": The by clause is parsed from PPL syntax by the ANTLR grammar, which only accepts valid field references. A user cannot pass an empty string as a partition key through normal PPL syntax (source=t | eventstats row_number() by "" would be a parser error or would reference a field named ""). This concern is not specific to this bugfix and is handled by the parser layer.

No code changes needed for these comments.

@qianheng-aws qianheng-aws marked this pull request as ready for review April 7, 2026 14:22
@songkant-aws
Copy link
Copy Markdown

Have duplicate fix now: opensearch-project#5305

@songkant-aws
Copy link
Copy Markdown

Ah, I see. Is this PR used for testing workflow?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] eventstats/streamstats reject window functions that grammar accepts

2 participants