Add support for FETCH FIRST WITH TIES clause by kasiafi · Pull Request #832 · trinodb/trino

kasiafi · 2019-05-28T13:07:20Z

Relates to #1

martint · 2019-05-31T16:05:22Z

...in/src/main/java/io/prestosql/sql/planner/iterative/rule/ImplementLimitWithTiesOverSort.java

Seeing this "non-trivial" rewrite that looks almost the same code as the other ones got me thinking about whether we're representing "with ties" properly in the plan. Let me describe my thought process and then suggest how we might want to do it:

The rewrites look similar and are not trivial so we should somehow share the implementation

At a fundamental level, what we're doing is look for a "limit with ties" over something that is "ordered"

One option would be to somehow tag the Sort / TopN node with a marker interface and a single method to get the ordering they compute. However, this wouldn't work for the case with project in between -- we'd still need separate rules.

Could we model this with traits (not supported yet, but...)? Unfortunately, this is not sufficient. We could attempt to match a "limit with ties" over "something that guarantees some kind of ordering". Traits might describe a tighter guarantee than what was originally in the ORDER BY clause. Imagine, for instance, a case where the data source is sorted by more columns than show up in the ORDER BY clause.

Which leads me to the following conclusion: "limit with ties" is not just a simple limit and a flag. The set of columns used to define the ties is meaningful. Conceptually one could think of FETCH FIRST vs ORDER BY ... FETCH FIRST WITH TIES as two completely different operations that happen to share some of the same syntax in the language.

And the suggestion on how we might approach it: add the ordering scheme to "limit with ties" (or create a new node for it). These rules would just find a "limit with ties" and rewrite into the new form without worrying about whether there's a sort or topN or anything else under them.

Which leads me to the following conclusion: "limit with ties" is not just a simple limit and a flag. The set of columns used to define the ties is meaningful.

Of course, FETCH FIRST ... WITH TIES must be aware of the exact ordering scheme from the corresponding ORDER BY clause. Adding the ordering scheme to the LimitNode would simplify things a lot.

However, that would be quite a different approach from how ORDER BY ... LIMIT is handled.
As I recall, LimitNode and SortNode are planned distinctly, possibly with an OffsetNode between them, and there are rules responsible for merging them into TopNNode.
I thought it was right to let QueryPlanner simply rewrite a query into a plan and let the rules take care of the implementation.

Maybe the analogy between 'with ties' and TopN isn't so close. Pairing a LimitNode with ordering scheme to create a TopNNode is a matter of optimisation, while in the case of 'with ties' it is a necessity.

Another thought.
If we are going to add the ordering scheme to LimitNode with ties, then why don't we add it to LimitNode without ties, too? I'm aware this idea doesn't comply with the principle of QueryPlanner being merely a translator AST -> plan. However, there are benefits:

simplicity at planning stage (no check for with ties)

easy transformation of LimitNode without ties into TopNNode (currently there are multiple rules to check if the source is sorted / project + sorted). Same as for LimitNode with ties.

maybe we could even quit planning Sort at all when Limit is present and there are no other sort-dependent clauses (Offset). LimitNode without ties would be transformed into TopNNode, and LimitNode with ties would be transformed into WindowNode with OrderingScheme, which subsequently would emit a distributed Sort for efficiency (or so I heard ;) )

Analyzer throws NOT_SUPPORTED exception.

kasiafi · 2019-06-08T20:53:42Z

@martint could you please review?

martint · 2019-06-10T05:27:24Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PruneLimitColumns.java

This could prune any columns that are not needed for breaking ties

martint · 2019-06-10T05:28:06Z

...main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushLimitThroughMarkDistinct.java

, is not needed.

martint · 2019-06-10T05:31:22Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushLimitThroughProject.java

Because I mistakenly assumed that Limit with ties would never match anyway.
I'll add it to the rule.
BTW if I was right, then what was the proper thing to do? Exclude it in the pattern? Or make the rule so that it takes account of all possibilities?

Excluding in the pattern is fine, assuming that's what we need to do.

martint · 2019-06-10T05:35:24Z

presto-main/src/main/java/io/prestosql/sql/planner/optimizations/AddExchanges.java

Why? Everything this method does should apply to limit with ties if it were ever encountered, no?

Yes, but I wanted to be sure that LimitNode with ties never reaches this point. The same in the other places you pointed out. Should I support LimitNode with ties there? Is there a way to say "no LimitNodes with ties behind this point"?

I guess it's fine to not support that here for now. I'm more concerned about other optimizers like UnaliasSymbolReferences, etc, which tend to scattered all over the place.

Could you please specify, in which optimizers I need to support Limit with ties? And whether other optimizers should throw exception.

UnaliasSymbolReferences and PruneUnreferencedOutputs and LimitPushdown (for now, since we're in the process of replacing it with Rules).

We should look into whether we need to do anything for WindowFilterPushdown.

martint · 2019-06-10T05:35:37Z

presto-main/src/main/java/io/prestosql/sql/planner/optimizations/AddLocalExchanges.java

martint · 2019-06-10T05:37:35Z

presto-main/src/main/java/io/prestosql/sql/planner/plan/LimitNode.java

These can all go on the same line

martint · 2019-06-10T05:38:50Z

presto-main/src/main/java/io/prestosql/sql/planner/plan/LimitNode.java

This is a derived property, so it shouldn't be part of the json serialization.

martint · 2019-06-10T05:40:22Z

presto-main/src/main/java/io/prestosql/sql/planner/sanity/ValidateDependenciesChecker.java

This check is already removed in the following commit where final implementation of LimitNode with ties is added.

Got it. I missed that when reviewing commits individually.

martint · 2019-06-10T05:46:19Z

presto-tests/src/main/java/io/prestosql/tests/AbstractTestQueries.java

Don't abbreviate variable names. It makes the code harder to read

martint · 2019-06-10T05:56:14Z

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/PlanPrinter.java

This renders as: Limit[10 withTies: ]. Did you mean to put the colon before withTies? Maybe do Limit[10+ties] instead.

This change adds withTies::boolean to LimitNode. Existing optimizer rules are changed and commented accordingly to LimitNode with ties expected behavior. Implementation of LimitNode with ties is not yet added, but IllegalStateException("Unexpected node") is thrown when LimitNode with ties is encountered past the point of its replacement.

This change adds an Optimizer rule to replace LimitNode having 'with ties' property. Also, Plan tests are added, and AbstractTestQueries to prove correct semantics and demonstrate how WITH TIES clause cooperates with ORDER BY and OFFSET clauses.

kasiafi · 2019-06-10T22:33:13Z

Applied comments.

martint · 2019-06-11T03:13:20Z

Good job! I made a couple of minor adjustments to a commit message and comment and merged it. Thanks!

cla-bot bot added the cla-signed label May 28, 2019

kasiafi force-pushed the FetchFirstWithTies branch from ba9e3c4 to 128425e Compare May 28, 2019 13:25

martint self-requested a review May 30, 2019 02:43

kasiafi force-pushed the FetchFirstWithTies branch from 128425e to 7a1d23d Compare May 30, 2019 13:18

martint reviewed May 31, 2019

View reviewed changes

kasiafi added 3 commits June 8, 2019 21:22

Support Offset in PlanPrinter

eafbfd8

Add support for FETCH FIRST clause WITH TIES in grammar and AST

4c25f71

Analyzer throws NOT_SUPPORTED exception.

Prune Offset outputs

fa73b06

kasiafi force-pushed the FetchFirstWithTies branch from 7a1d23d to 29e7dc0 Compare June 8, 2019 19:34

martint reviewed Jun 10, 2019

View reviewed changes

kasiafi added 2 commits June 10, 2019 22:44

Implement FETCH FIRST WITH TIES

bf2affc

This change adds an Optimizer rule to replace LimitNode having 'with ties' property. Also, Plan tests are added, and AbstractTestQueries to prove correct semantics and demonstrate how WITH TIES clause cooperates with ORDER BY and OFFSET clauses.

kasiafi force-pushed the FetchFirstWithTies branch from 29e7dc0 to bf2affc Compare June 10, 2019 20:45

martint approved these changes Jun 11, 2019

View reviewed changes

martint closed this Jun 11, 2019

martint added this to the 315 milestone Jun 11, 2019

martint mentioned this pull request Jun 11, 2019

Release notes for 315 #948

Closed

findepi mentioned this pull request Jun 11, 2019

Add support for FETCH/OFFSET syntax #1

Closed

fgwang7w mentioned this pull request Jul 22, 2020

Add support for FETCH clause and FETCH FIRST WITH TIES clause prestodb/presto#14871

Closed

Conversation

kasiafi commented May 28, 2019 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi commented Jun 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi commented Jun 10, 2019

Uh oh!

martint commented Jun 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

kasiafi commented May 28, 2019 •

edited by findepi

Loading

kasiafi May 31, 2019 •

edited

Loading

kasiafi May 31, 2019 •

edited

Loading

kasiafi Jun 10, 2019 •

edited

Loading