feat(sql): Support RANK/DENSE_RANK for unified SQL

[dai-chen]

Description

RANK() and DENSE_RANK() were already accepted by the SQL grammar and declared as built-in function names, but were never registered as window functions, so planning failed with Unexpected window function: RANK. This PR registers both and maps them to Calcite's standard RANK and DENSE_RANK operators. No grammar, lexer or AST changes were needed.

Related Issues

Part of #5248

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 544e467)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 544e467

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Explicitly pass null for frame bounds Since RANK and DENSE_RANK disallow framing, passing lowerBound and upperBound parameters may cause unexpected behavior or errors. Consider passing null for both bounds to explicitly indicate no framing is used. core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java [236-251] case RANK: return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.RANK), partitions, orderKeys, false, - lowerBound, - upperBound); + null, + null); case DENSE_RANK: return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.DENSE_RANK), partitions, orderKeys, false, - lowerBound, - upperBound); + null, + null); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that RANK and DENSE_RANK disallow framing, and the comment in the code confirms this. Passing null instead of lowerBound and upperBound would make the intent more explicit and prevent potential issues, though the current implementation may already handle this correctly through normalization.	Medium

---

Previous suggestions

Suggestions up to commit 77ae6e0

Category	Suggestion	Impact
Possible issue	Explicitly pass null for frame bounds Since RANK and DENSE_RANK disallow framing, passing lowerBound and upperBound parameters may cause unexpected behavior or errors. Consider explicitly passing null for both bounds to ensure proper handling of these ranking functions. core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java [236-251] case RANK: return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.RANK), partitions, orderKeys, false, - lowerBound, - upperBound); + null, + null); case DENSE_RANK: return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.DENSE_RANK), partitions, orderKeys, false, - lowerBound, - upperBound); + null, + null); Suggestion importance[1-10]: 4 __ Why: While the suggestion correctly identifies that RANK and DENSE_RANK disallow framing, the comment in the PR already acknowledges this ("Calcite rank operators disallow framing, so the ROWS/RANGE flag below is normalized away"). The current implementation passes lowerBound and upperBound which are likely normalized by Calcite. Explicitly passing null would be slightly clearer but is not critical since the normalization handles this.	Low

Suggestions up to commit 747d1b6

Category	Suggestion	Impact
Possible issue	Validate ORDER BY clause presence The RANK and DENSE_RANK window functions require an ORDER BY clause to function correctly. Consider validating that orderKeys is not empty before creating the aggregate call to prevent runtime errors or unexpected behavior when no ordering is specified. core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java [236-251] case RANK: + if (orderKeys.isEmpty()) { + throw new IllegalArgumentException("RANK requires ORDER BY clause"); + } return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.RANK), partitions, orderKeys, false, lowerBound, upperBound); case DENSE_RANK: + if (orderKeys.isEmpty()) { + throw new IllegalArgumentException("DENSE_RANK requires ORDER BY clause"); + } return withOver( context.relBuilder.aggregateCall(SqlStdOperatorTable.DENSE_RANK), partitions, orderKeys, false, lowerBound, upperBound); Suggestion importance[1-10]: 5 __ Why: While RANK and DENSE_RANK do require an ORDER BY clause semantically, this validation may already be handled at the SQL parsing/validation layer. Adding redundant validation here could be useful for defensive programming, but without evidence of missing validation upstream, this is a moderate improvement for robustness rather than fixing a critical bug.	Low

Suggestions up to commit 352ffbd

Category	Suggestion	Impact
General	Use Set for multiple condition checks Consider using a Set or EnumSet for checking multiple function names instead of chained OR conditions. This improves readability and makes it easier to add more functions in the future. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [775-777] -if (functionName == BuiltinFunctionName.ROW_NUMBER - || functionName == BuiltinFunctionName.RANK - || functionName == BuiltinFunctionName.DENSE_RANK) { +private static final Set<BuiltinFunctionName> NO_FIELD_WINDOW_FUNCTIONS = + EnumSet.of(BuiltinFunctionName.ROW_NUMBER, BuiltinFunctionName.RANK, BuiltinFunctionName.DENSE_RANK); +if (NO_FIELD_WINDOW_FUNCTIONS.contains(functionName)) { + Suggestion importance[1-10]: 4 __ Why: While using an EnumSet would improve maintainability and readability, the current chained OR condition with three items is still acceptable and clear. The suggestion is valid but offers only a moderate improvement in code style rather than fixing a critical issue.	Low

[github-actions]

Persistent review updated to latest commit 747d1b6

[github-actions]

Persistent review updated to latest commit 77ae6e0

[github-actions]

Persistent review updated to latest commit 544e467

[RyanL1997]

LGTM. Just would like to double check do we need to add doc test/doc for this?

[RyanL1997]

In addtion, I found out that

The following isn't something you introduced in this PR. The existing ROW_NUMBER case has the same shape, and ROW_NUMBER() OVER (ORDER BY age) already returns 1 for every row. The new RANK/DENSE_RANK cases inherit it.

Behaviour:

SELECT age, RANK() OVER (ORDER BY age) FROM employees
  actual   → 4, 4, 4, 4      (the partition size)
  expected → 1, 2, 3, 4

Why: the new cases forward the caller's lowerBound/upperBound unchanged, and for SQL those default to WindowFrame.rowsUnbounded() = UNBOUNDED PRECEDING … UNBOUNDED FOLLOWING, so every row is ranked over the whole partition. Calcite's own SqlToRelConverter.convertOver forces UNBOUNDED PRECEDING … CURRENT ROW for operators with allowsFraming() == false; this branch needs to do the same.

One knock-on: the plan assertions can't catch it. allowsFraming() only suppresses the frame in the printed digest, not in the executed Window.Group — so RANK() OVER (ORDER BY $2 NULLS FIRST) prints identically whichever frame was built. Confirming this needs an execution-level assertion.