Skip to content

Add UPDATE sql support in Presto Engine#21435

Merged
yingsu00 merged 1 commit intoprestodb:masterfrom
agrawalreetika:update-support
Dec 4, 2023
Merged

Add UPDATE sql support in Presto Engine#21435
yingsu00 merged 1 commit intoprestodb:masterfrom
agrawalreetika:update-support

Conversation

@agrawalreetika
Copy link
Member

@agrawalreetika agrawalreetika commented Nov 22, 2023

Description

Add UPDATE SQL support in Presto Engine.
PR also includes new developer documentation which has details about how it works
Cherry-pick of trinodb/trino@af17e51

Motivation and Context

#20571

Impact

  • New SQL Support

Test Plan

  • Tested Parser changes and added tests for the same
  • Once we add support for UPDATE for any connector, then complete flow could be tested via Product test added in AbstractTestDistributedQueries.java

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Add UPDATE sql support in Presto Engine

@agrawalreetika agrawalreetika marked this pull request as ready for review November 23, 2023 06:20
@agrawalreetika agrawalreetika requested a review from a team as a code owner November 23, 2023 06:20
@agrawalreetika agrawalreetika changed the title [WIP] Add UPDATE sql support in Presto Engine Add UPDATE sql support in Presto Engine Nov 23, 2023
@github-actions
Copy link

github-actions bot commented Nov 23, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff e2e22b3...c4b715f.

Notify File(s)
@aditi-pandit presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@elharo presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@kaikalur presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@rschlussel presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@steveburnett presto-docs/src/main/sphinx/develop.rst
presto-docs/src/main/sphinx/develop/delete-and-update.rst
presto-docs/src/main/sphinx/sql.rst
presto-docs/src/main/sphinx/sql/update.rst

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

This is great doc, thanks!

I made some rephrasing and formatting suggestions, some of which are only suggestions for you to consider if you think they help. As always, if I've misunderstood your meaning, help me understand and we will find a solution.

(Oh, there are no comments about sql/update.rst because I didn't have anything to say about that file. I'm just mentioning it here to say that I didn't miss it :) ).

Copy link
Contributor

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

@agrawaldevesh Was this ported from Trino? If yes can you please add link to the Trino commit/PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just inline table

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add empty line above

@yingsu00
Copy link
Contributor

@agrawalreetika There are test failures

Error:  Tests run: 192, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 1.728 s <<< FAILURE! - in TestSuite
Error:  com.facebook.presto.sql.parser.TestSqlParserErrorHandling.testStatement[, line 1:1: mismatched input '<EOF>'. Expecting: 'ALTER', 'ANALYZE', 'CALL', 'COMMIT', 'CREATE', 'DEALLOCATE', 'DELETE', 'DESC', 'DESCRIBE', 'DROP', 'EXECUTE', 'EXPLAIN', 'GRANT', 'INSERT', 'PREPARE', 'REFRESH', 'RESET', 'REVOKE', 'ROLLBACK', 'SET', 'SHOW', 'START', 'UPDATE', 'TRUNCATE', 'USE', <query>](1)  Time elapsed: 0.004 s  <<< FAILURE!

@agrawalreetika agrawalreetika force-pushed the update-support branch 3 times, most recently from a8581db to 3bac874 Compare November 28, 2023 14:17
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Thanks!

@agrawalreetika
Copy link
Member Author

@yingsu00, I had added Trino commit in the commit message, I have now added it to PR description as well.
I have fixed the tests as well, please review.

@tdcmeehan tdcmeehan linked an issue Nov 28, 2023 that may be closed by this pull request
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

@agrawalreetika agrawalreetika Nov 29, 2023

Choose a reason for hiding this comment

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

It is required to resolve imports in UpdateNode Class

Copy link
Contributor

Choose a reason for hiding this comment

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

@agrawalreetika I just realized you added the UpdateNode in SPI. I think it should be in com.facebook.presto.sql.planner.plan package, not the com.facebook.presto.spi.plan package. It should be the same as where DeleteNode is. com.facebook.presto.sql.planner.plan is for the plan nodes that are invisible to connectors and not modifiable by the connectors. Iceberg connector is not supposed to modify this PlanNode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes @yingsu00, I noticed this refactor was done in Presto. So I have also moved UpdateNode as well in same presto-main module itself.

Cherry-pick of trinodb/trino@af17e51

Co-authored-by: djsstarburst
@yingsu00
Copy link
Contributor

Reetika, UpdatablePageSource was removed in trinodb/trino#15161. Will you be able to check if it's applicable to Presto? Iceberg does not use this page source.

@agrawalreetika agrawalreetika self-assigned this Nov 30, 2023
@yingsu00 yingsu00 merged commit 76ae3ed into prestodb:master Dec 4, 2023
@wanglinsong wanglinsong mentioned this pull request Feb 12, 2024
64 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SQL Support for UPDATE In Presto

3 participants