Conversation
kasiafi
force-pushed
the
OffsetImplementation
branch
from
0fee17d to
6f8e3d8
Compare
martint
left a comment
martint
left a comment
There was a problem hiding this comment.
Looks great! Just a few minor comments.
There was a problem hiding this comment.
Just call these PROJECT and SORT? I don't think the _CHILD part adds any information. In fact, it's slightly confusing as it leads the reader to think it might refer to the "child of the project node".
There was a problem hiding this comment.
This should check that offset >= 0. Even though the parser guarantees the value will be >= 0, the API to the analyzer is an AST, which could be constructed by hand with an invalid offset.
There was a problem hiding this comment.
Same here, just name it PROJECT and SORT
There was a problem hiding this comment.
Rename these to project and sort
There was a problem hiding this comment.
Rename these to project and sort. The same applies to all the other rules.
There was a problem hiding this comment.
This is not guaranteed to produce 'C', 'B' since there's no ordering. Verify this query by computing the results and ensuring it contains:
- 2 rows
- they are different from each other
- they are contained in the original set of values
See testLimit in this class as an example.
This rule merges LimitNode with SortNode into TopNNode in the case when there is identity ProjectNode between LimitNode and SortNode.
kasiafi
force-pushed
the
OffsetImplementation
branch
from
6f8e3d8 to
a469575
Compare
|
Applied comments. |
|
Merged, thanks! |
Cherry-pick of trinodb/trino#732, trinodb/trino#820 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Cherry-pick of trinodb/trino#732, trinodb/trino#820 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Cherry-pick of trinodb/trino#732, trinodb/trino#820 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
2 participants
Relates to #1