Introduce new Keyword FETCH FIRST N ROWS ONLY in Presto Query

[skairali]

Introduce new Keyword FETCH FIRST N ROWS ONLY in Presto Query
Issue details : #18935

We are introducing a new keyword FETCH FIRST N ROWS ONLY which can be used in presto queries. The keyword will work in sql queries just like LIMIT keyword to limit the number of rows returned from an sql select query.

For Example : select * from table FETCH FIRST 3 ROWS ONLY will return only 3 rows as a result of the sql query.

== RELEASE NOTES ==

General Changes

Introducing a new Keyword FETCH FIRST N ROWS ONLY to the Presto Query

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bentonyjoe191 (ea64fb6)
• ✅ login: skairali / name: Sudheesh (8c40406, b3c8716)

[rschlussel]

Thanks for the contribution! I didn't realize until now that "LIMIT" wasn't in the sql standard.

FYI @jainxrohit @elharo - this will need to be added to crux as well.

[rschlussel]

offset comes before "FETCH FIRST"

[skairali]

This is addressed

[rschlussel]

we shouldn't allow queries to have both LIMIT and FETCH FIRST

[skairali]

This is handled and necessary conditions are in place

[elharo]

Why do we want this and LIMIT?

Where does the syntax come from? This seems quite verbose compared to LIMIT and is more than one keyword, or perhaps not a keyword at all. I'd like a much more complete rationale for this change before accepting it.

[rschlussel]

Syntax comes from the SQL Spec, which is why I think we should support it. It turns out LIMIT isn't actually in the SQL Spec even though i assume it's way more widely used.

From 2016 sql spec

<query expression> ::=
  [ <with clause> ] <query expression body>
      [ <order by clause> ] [ <result offset 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

[rschlussel]

You can read more about the all the various ways of limiting result rows across DBs here: https://en.wikipedia.org/wiki/Select_(SQL)#Limiting_result_rows.

[kaikalur]

Or you could just get the whole SQL 2016 spec using my awesome SQL 16 parser :)

https://github.com/prestodb/sql/

[elharo]

I think this needs a broader discussion amongst the interested parties before committing. Standard or otherwise, we already have LIMIT. Adding a new way of doing the same thing is worth a second look. I don't think Presto anyone else have ever promised to implement the full ISO spec.

[rschlussel]

I think this needs a broader discussion amongst the interested parties before committing. Standard or otherwise, we already have LIMIT. Adding a new way of doing the same thing is worth a second look. I don't think Presto anyone else have ever promised to implement the full ISO spec.

We don't promise to implement the full ISO spec, but for features we have, we strive to implement them in a way that is ISO SQL compliant. As a general principle, I agree it's good to have just one way of doing things. But in this case, the cost of this change is very minimal (very limited in scope), and it brings us better SQL compliance. I don't see a big downside here.

[skairali]

@rschlussel Exactly, While fetch and limit have the same functionality, the fetch clause is an SQL standard while limit is not. Fetch was included in the SQL standard in 2008 which makes it the go to option if you want your query to be portable across several RDBMS. So this is a MUST needed in terms of SQL compatibility standpoint for all presto consumers

[skairali]

@rschlussel Please see that your comments are addressed.

[v-jizhang]

@prestobot kick off tests

[rschlussel]

Had offline discussion with @elharo, and he no longer has concerns about this change.

[rschlussel]

@skairali sorry, i merged this without noticing, but in the future, commits should be self contained, so squash all the "address comments" type of commits in to the original, and also always rebase changes on latest rather than merging. We don't like to have merge commits

[skairali]

@rschlussel Sure

[rohanpednekar]

@skairali, can you pls add documentation PR about this new feature? I think this should be listed here as well. Thanks!