Add support for FETCH clause and FETCH FIRST WITH TIES clause by fgwang7w · Pull Request #14871 · prestodb/presto

fgwang7w · 2020-07-22T04:41:10Z

Syntax:

<query expression> ::=
  [ <with clause> ] <query expression body>
      [ <order by clause> ] [ <fetch first clause> ]

<fetch first clause> ::=
  FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }

<fetch first quantity> ::=
    <fetch first row count>
  | <fetch first percentage>

<fetch first row count> ::=
  <simple value specification>

<fetch first percentage> ::=
  <simple value specification> PERCENT

Extracted-From:
trinodb/trino#666
trinodb/trino#832

Test plan - Add UT and test locally

== RELEASE NOTES ==

General Changes
* Add support for FETCH syntax 
* Add support for FETCH FIRST WITH TIES syntax

lucaskim1233 · 2020-07-23T08:49:55Z

Is this feature scheduled for next release?

kaikalur

LGTM

fgwang7w · 2020-07-23T17:07:41Z

LGTM

Thanks for reviewing. However, current commit only suppport FETCH syntax and OFFSET syntax. Add support for FETCH FIRST WITH TIES clause has not yet been committed. OFFSET implementation is still being worked on.
The code base is quite complicated and direct backporting is not possible which now results in many handy maneuvers. Is it acceptable to include suppport for FETCH syntax and OFFSET syntax only in the next release and another commit in the subsequent release? @kaikalur fyi @rongrong

fgwang7w · 2020-07-24T21:52:36Z

@kaikalur I added the implementation for OFFSET and add support for FETCH FIRST WITH TIES clause. Could you please review again. I'd like to retain 2 commits as the 2nd commit contains the support for FETCH FIRST WITH TIES clause. Thanks.

rongrong · 2020-07-28T23:21:33Z

LGTM

Thanks for reviewing. However, current commit only suppport FETCH syntax and OFFSET syntax. Add support for FETCH FIRST WITH TIES clause has not yet been committed. OFFSET implementation is still being worked on.
The code base is quite complicated and direct backporting is not possible which now results in many handy maneuvers. Is it acceptable to include suppport for FETCH syntax and OFFSET syntax only in the next release and another commit in the subsequent release? @kaikalur fyi @rongrong

I'm OK with leaving FETCH FIRST WITH TIES into a different PR if it's quite complicated. Please update the description accordingly if you decided to do so. The test failures seem to be related. Please fix them. Thanks!

fgwang7w · 2020-07-29T06:57:58Z

LGTM

Thanks for reviewing. However, current commit only suppport FETCH syntax and OFFSET syntax. Add support for FETCH FIRST WITH TIES clause has not yet been committed. OFFSET implementation is still being worked on.
The code base is quite complicated and direct backporting is not possible which now results in many handy maneuvers. Is it acceptable to include suppport for FETCH syntax and OFFSET syntax only in the next release and another commit in the subsequent release? @kaikalur fyi @rongrong

I'm OK with leaving FETCH FIRST WITH TIES into a different PR if it's quite complicated. Please update the description accordingly if you decided to do so. The test failures seem to be related. Please fix them. Thanks!

Acutually FETCH FIRST TIES support is included in the same PR now. I am working on fixing the test failures now.

fgwang7w · 2020-08-05T00:17:46Z

All tests fixed. Please help review the code for next release. Thanks!

aweisberg · 2020-09-29T18:29:26Z

Was there code in this PR copied from another project? If so please follow the attribution guidelines.

aweisberg · 2020-10-06T14:51:09Z

Just FYI it seems like 3/4 of the commits don't have a co-author tag. Thanks!

fgwang7w · 2020-10-07T23:30:27Z

Just FYI it seems like 3/4 of the commits don't have a co-author tag. Thanks!

commit msg is fixed

fgwang7w · 2020-10-07T23:32:12Z

@kaikalur had an LGTM on Jul 23, please review again. @tdcmeehan please help move forward for this PR. this has been waiting for too long to get merged since Jul 23

kaikalur · 2020-10-08T02:13:31Z

So I'm still confused - can we create 4 separate PRs? That'll be easier to review/track.

fgwang7w · 2020-10-08T02:23:10Z

So I'm still confused - can we create 4 separate PRs? That'll be easier to review/track.

I think it's already splitted into 4 commits which can be reviewed individually as Tim did. are you suggesting to split those 4 commits into 4 PRs now?

kaikalur · 2020-10-08T18:21:54Z

So I'm still confused - can we create 4 separate PRs? That'll be easier to review/track.

I think it's already splitted into 4 commits which can be reviewed individually as Tim did. are you suggesting to split those 4 commits into 4 PRs now?

I think that will be better.

kaikalur · 2020-10-08T18:49:43Z

So I'm still confused - can we create 4 separate PRs? That'll be easier to review/track.

I think it's already splitted into 4 commits which can be reviewed individually as Tim did. are you suggesting to split those 4 commits into 4 PRs now?

I think that will be better.

So chatted with Rongrong offline and it's ok to have just one PR with 4 commits. I will review them before Monday.

kaikalur

For the fist commit.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

presto-main/src/main/java/com/facebook/presto/sql/rewrite/DescribeInputRewrite.java

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java

NikhilCollooru · 2021-04-22T19:05:19Z

Hi @fgwang7w can you please address the comments and update the PR ?

We need OFFSET feature and hence we need this PR . Can you please fix the PR ?

fgwang7w · 2021-05-02T03:07:05Z

Hi @fgwang7w can you please address the comments and update the PR ?

We need OFFSET feature and hence we need this PR . Can you please fix the PR ?

Supoprt for OFFSET clause is now implemented via this PR #16035

Cherry-pick of trinodb/trino@a9e7145 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

Cherry-pick of trinodb/trino315 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

kaikalur

I put a couple of comments. But my high level comment would be to keep this PR non-intrusive. I see a lot of core planner changes. I'm not sure why those are needed.

kaikalur · 2021-07-19T22:20:14Z

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java

                                new ImplementBernoulliSampleAsFilter(),
-                                new ImplementOffset())),
+                                new ImplementOffset(),
+                                new ImplementLimitWithTies(metadata))),


We need this to be controlled by a session param/feature config. In fact, I would like the whole feature to be optional.

kaikalur · 2021-07-19T22:21:14Z

presto-main/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java

    {
        if (!orderBy.isPresent() || (isSkipRedundantSort(session)) && analysis.isOrderByRedundant(orderBy.get())) {
-            return subPlan;
+            return Optional.empty();


Why do we need this change?

stale · 2022-03-02T09:23:49Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

rongrong requested a review from kaikalur July 22, 2020 22:45

kaikalur approved these changes Jul 23, 2020

View reviewed changes

fgwang7w force-pushed the 14837 branch 2 times, most recently from 9a1cbfd to 12f8494 Compare July 24, 2020 21:49

fgwang7w force-pushed the 14837 branch from 12f8494 to 756ee66 Compare July 25, 2020 00:21

fgwang7w changed the title ~~Add support for FETCH syntax and OFFSET syntax~~ Add support for FETCH, OFFSET, and TIES syntax to an enhanced LIMIT capability Jul 25, 2020

fgwang7w force-pushed the 14837 branch from 756ee66 to 6540141 Compare July 25, 2020 06:16

rongrong self-requested a review July 28, 2020 23:21

fgwang7w force-pushed the 14837 branch from 6540141 to 6507a4c Compare July 29, 2020 06:22

fgwang7w force-pushed the 14837 branch 2 times, most recently from d266b0e to ec5bb8f Compare July 29, 2020 22:09

jagill mentioned this pull request Jul 29, 2020

Add geometry_nearest_points function #14923

Merged

fgwang7w force-pushed the 14837 branch 11 times, most recently from 30bd260 to 476dae2 Compare August 4, 2020 04:26

fgwang7w force-pushed the 14837 branch from 476dae2 to 76379c8 Compare August 12, 2020 06:21

fgwang7w force-pushed the 14837 branch from 6947617 to 758c293 Compare September 28, 2020 20:57

fgwang7w force-pushed the 14837 branch from 758c293 to 5f1bec5 Compare October 7, 2020 23:26

fgwang7w requested a review from tdcmeehan October 7, 2020 23:32

kaikalur requested changes Oct 12, 2020

View reviewed changes

fgwang7w force-pushed the 14837 branch 2 times, most recently from 551906c to 9f8cd43 Compare May 2, 2021 02:45

fgwang7w changed the title ~~Enhance LIMIT capability: add support for FETCH, OFFSET, FETCH FIRST WITH TIES syntax~~ Add support for FETCH clause and FETCH FIRST WITH TIES clause May 2, 2021

fgwang7w force-pushed the 14837 branch 2 times, most recently from 38247b8 to da42dcb Compare May 4, 2021 16:57

fgwang7w requested a review from kaikalur May 5, 2021 21:56

fgwang7w force-pushed the 14837 branch 3 times, most recently from d3b4a75 to 97b8737 Compare May 21, 2021 00:29

fgwang7w and others added 5 commits June 26, 2021 22:02

Add support for FETCH syntax

975f7e9

Cherry-pick of trinodb/trino@a9e7145 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

revision for FETCH implementation

9373d10

Add support for FETCH FIRST clause WITH TIES(allowing syntax)

7ae4dfa

Cherry-pick of trinodb/trino315 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

Add optimizer rule to eanble LimitNode having 'With Ties' property

be32619

Cherry-pick of trinodb/trino315 Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

revision for FETCH

a37d692

kaikalur requested changes Jul 19, 2021

View reviewed changes

Conversation

fgwang7w commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucaskim1233 commented Jul 23, 2020

Uh oh!

kaikalur left a comment

Choose a reason for hiding this comment

Uh oh!

fgwang7w commented Jul 23, 2020

Uh oh!

fgwang7w commented Jul 24, 2020

Uh oh!

rongrong commented Jul 28, 2020

Uh oh!

fgwang7w commented Jul 29, 2020

Uh oh!

fgwang7w commented Aug 5, 2020

Uh oh!

aweisberg commented Sep 29, 2020

Uh oh!

aweisberg commented Oct 6, 2020

Uh oh!

fgwang7w commented Oct 7, 2020

Uh oh!

fgwang7w commented Oct 7, 2020

Uh oh!

kaikalur commented Oct 8, 2020

Uh oh!

fgwang7w commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaikalur commented Oct 8, 2020

Uh oh!

kaikalur commented Oct 8, 2020

Uh oh!

kaikalur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikhilCollooru commented Apr 22, 2021

Uh oh!

fgwang7w commented May 2, 2021

Uh oh!

kaikalur left a comment

Choose a reason for hiding this comment

Uh oh!

kaikalur Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

kaikalur Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fgwang7w commented Jul 22, 2020 •

edited

Loading

fgwang7w commented Oct 8, 2020 •

edited

Loading