Skip to content

[CALCITE-7644] Window ORDER BY expression is unparsed as positional ordinal#5081

Open
snuyanzin wants to merge 1 commit into
apache:mainfrom
snuyanzin:calcite7644
Open

[CALCITE-7644] Window ORDER BY expression is unparsed as positional ordinal#5081
snuyanzin wants to merge 1 commit into
apache:mainfrom
snuyanzin:calcite7644

Conversation

@snuyanzin

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7644

Changes Proposed

in case of OVER(...) window's ORDER BY should be passed as a field expression rather than positional ordinal (also see jira issue description)

@sonarqubecloud

sonarqubecloud Bot commented Jul 5, 2026

Copy link
Copy Markdown

for (RelFieldCollation collation : group.orderKeys.getFieldCollations()) {
this.addOrderItem(orderByKeys, collation);
// A window ORDER BY has no ordinal notion; resolve via field().
this.addOrderItem(orderByKeys, collation, false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this branch isn't covered by tests. Could you show the execution results for the case where the value is false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't get what you mean by that, can you elaborate please?

This is exactly the case covered by testWindowOrderByExpression in this PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see—regarding the addOrderItem method, you added a parameter called allowsOrdinal. The case statement seems to cover the scenario where allowsOrdinal is true; if I’m not mistaken, shouldn't there also be a case for when it is false?

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants