Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[native] Fix 'Number of sorting keys must be greater than zero' #22050

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

mbasmanova
Copy link
Contributor

A query with row_number() window function using same partitioning and sorting
keys and a limit on row number values used to fail with

Caused by: java.lang.RuntimeException: sortingKeys_.size() > 0 (0 vs. 0) Number of sorting keys must be greater than zero

This happened because Prestissimo dropped sorting keys from Presto's
TopNRowNumberNode and tried to create TopNRowNumberNode in Velox with no
sorting keys. The fix is to use Velox's RowNumberNode when all sorting keys are
present as partitioning keys.

A query with row_number() window function using same partitioning and sorting
keys and a limit on row number values used to fail with

```
Caused by: java.lang.RuntimeException: sortingKeys_.size() > 0 (0 vs. 0) Number of sorting keys must be greater than zero
```

This happened because Prestissimo dropped sorting keys from Presto's
TopNRowNumberNode and tried to create TopNRowNumberNode in Velox with no
sorting keys. The fix is to use Velox's RowNumberNode when all sorting keys are
present as partitioning keys.
@mbasmanova mbasmanova requested a review from a team as a code owner February 29, 2024 22:33
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @mbasmanova

Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @mbasmanova

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@mbasmanova thanks for the fix!

@mbasmanova mbasmanova merged commit cec0968 into prestodb:master Feb 29, 2024
58 of 59 checks passed
@mbasmanova mbasmanova deleted the fix-row-number branch February 29, 2024 23:41
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
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.

4 participants