Skip to content

Simplify OFFSET implementation#820

Merged
martint merged 1 commit intotrinodb:masterfrom
kasiafi:OffsetImplementation
May 26, 2019
Merged

Simplify OFFSET implementation#820
martint merged 1 commit intotrinodb:masterfrom
kasiafi:OffsetImplementation

Conversation

@kasiafi
Copy link
Member

@kasiafi kasiafi commented May 26, 2019

This change is based on RowNumberNode's property of keeping order of its input.
In OFFSET implementation, a RowNumberNode and a FilterNode are used for rows selection.
Before this change, if there was ORDER BY in the query, it was handled by one of
the specialised rules which ensured that sorting was replicated over RowNumberNode.
Since RowNumberNode respects sorted order of its input, it is unnecessary to add
a SortNode.

@cla-bot cla-bot bot added the cla-signed label May 26, 2019
Copy link
Member

Choose a reason for hiding this comment

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

Parenthesis not needed?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe put this one in the same group as ImplementBernoulliSampleAsFilter

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary parenthesis?

Copy link
Member

Choose a reason for hiding this comment

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

Same here

This change is based on RowNumberNode's property of keeping order of its input.
In OFFSET implementation, a RowNumberNode and a FilterNode are used for rows selection.
Before this change, if there was ORDER BY in the query, it was handled by one of
the specialised rules which ensured that sorting was replicated over RowNumberNode.
Since RowNumberNode respects sorted order of its input, it is unnecessary to add
a SortNode.
@kasiafi kasiafi force-pushed the OffsetImplementation branch from 54cfeed to 2fec4bd Compare May 26, 2019 18:33
@martint martint merged commit 819dc06 into trinodb:master May 26, 2019
@martint
Copy link
Member

martint commented May 26, 2019

Merged, thanks!

fgwang7w added a commit to fgwang7w/presto that referenced this pull request Jun 9, 2021
Cherry-pick of trinodb/trino#732, trinodb/trino#820

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
rongrong pushed a commit to prestodb/presto that referenced this pull request Jun 22, 2021
Cherry-pick of trinodb/trino#732, trinodb/trino#820

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
jzzhaofb-zz pushed a commit to jzzhaofb-zz/presto that referenced this pull request Jun 29, 2021
Cherry-pick of trinodb/trino#732, trinodb/trino#820

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants