[CALCITE-7644] Window ORDER BY expression is unparsed as positional ordinal#5081
[CALCITE-7644] Window ORDER BY expression is unparsed as positional ordinal#5081snuyanzin wants to merge 1 commit into
ORDER BY expression is unparsed as positional ordinal#5081Conversation
| 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); |
There was a problem hiding this comment.
It seems this branch isn't covered by tests. Could you show the execution results for the case where the value is false?
There was a problem hiding this comment.
didn't get what you mean by that, can you elaborate please?
This is exactly the case covered by testWindowOrderByExpression in this PR
There was a problem hiding this comment.
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?
|
|
Since Mihai has already approved it, I think you can go ahead and merge it. |
Jira Link
CALCITE-7644
Changes Proposed
in case of
OVER(...)window'sORDER BYshould be passed as a field expression rather than positional ordinal (also see jira issue description)