Support SQL UPDATE in the Presto Engine and Hive ACID Tables #5861

djsstarburst · 2020-11-06T15:20:24Z

This PR consists of two commits. The first commit adds support for SQL UPDATE in the Presto engine. That commit also includes a new connector developer doc delete-and-update.rst that explains in details how both DELETE and UPDATE work, and documents the APIs that the connector developer must implement.

@martint, please take a close look at the QueryPlanner changes to support UPDATE. I think I got it right, but you are the expert.

martint

Some initial quick comments.

presto-main/src/main/java/io/prestosql/sql/analyzer/StatementAnalyzer.java

presto-spi/src/main/java/io/prestosql/spi/connector/UpdateKind.java

presto-main/src/main/java/io/prestosql/sql/analyzer/StatementAnalyzer.java

presto-main/src/main/java/io/prestosql/sql/planner/LogicalPlanner.java

presto-main/src/main/java/io/prestosql/sql/planner/PlanBuilder.java

presto-parser/src/main/java/io/prestosql/sql/tree/Update.java

presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4

djsstarburst · 2020-11-07T01:18:47Z

Thanks for the great tips, @martint! I've force-pushed an update with your suggested changes.

abhishekkhanna1 · 2020-11-12T19:46:37Z

Hi @djsstarburst I have checked it out locally and running tests manually to do some sanity testing through my local setup for different scenarios. Will try to see if I can figure out any corner cases. Also going through the code and debugging to familiarize with the flow and the code structure so that going forward I can help with the Merge changes.

electrum

Done with the first commit, except for the developer documentation

presto-docs/src/main/sphinx/develop.rst

presto-docs/src/main/sphinx/connector/hive.rst

presto-main/src/main/java/io/prestosql/metadata/Metadata.java

presto-main/src/main/java/io/prestosql/operator/UpdateOperator.java

presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java

presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java

presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java

presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4

djsstarburst · 2020-11-19T04:41:44Z

Thanks very much for the close and tasteful review of the first commit, @electrum! I force-pushed all your suggested changes except for validation of projected symbols in UpdateNode, and the discussion of whether HiveColumnHandle.isHidden() columns should be removed from the list of eligible UPDATE SET targets.

presto-docs/src/main/sphinx/develop/delete-and-update.rst

abhishekkhanna1 · 2020-11-19T09:47:35Z

presto-docs/src/main/sphinx/develop/delete-and-update.rst

+  All column lists are in table column order, as are the ``updateExpressionChannelNumbers``.  The size of
+  ``updateExpressionChannelNumbers`` is one greater than the number of updated columns - - the last element of the
+  list is the channel number of the block of rowIds for the page, constructed from the rowId column handle.
+
+  ``beginUpdate()`` performs any orchestration needed in the connector to start processing the ``UPDATE``.
+  This orchestration varies from connector to connector.  In the Hive ACID connector, for example, ``beginUpdate()``
+  starts the Hive Metastore transaction; checks that the updated table is transactional and that neither


Might not be relevant, just thinking out loud. What would be the behavior if there is a concurrent Alter table which drops or renames the columns before the beginUpdate is invoked?

It's a good question, and an area that is poorly tested. If you could think of a way to test commit and rollback it would be a big contribution. Here is the way I think it works:

If the ALTER TABLE transaction committed before beginUpdate() was called, the UPDATE operation would fail because the StatementAnalyzer would fail to find a referenced column. If the ALTER TABLE transaction was started but not committed by the time beginUpdate() was called, I think the call to metastore.declareIntentionToWrite() will cause a rollback of the beginUpdate() transaction when the ALTER TABLE transaction commits. But I'm guessing, and don't have tests to prove that it works that way :(

Assuming the metastore was a proper transactional system that implemented optimistic concurrency, the commit of the UPDATE operation would fail due to the conflict. That's how it will work for Iceberg, which properly supports transaction semantics.

Makes sense that as long as we have OCC backed metastore, one of the conflicting transaction would be rejected. But, I was thinking can there be an Alter table request which makes the conflict not detectable because we rely on the names/order of the columns? e.g the scenarios listed here https://iceberg.apache.org/evolution/#correctness

Formats that track columns by name can inadvertently un-delete a column if a name is reused, which violates #1. Formats that track columns by position cannot delete columns without changing the names that are used for each column, which violates #2.

presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergOrcFileWriter.java

presto-main/src/main/java/io/prestosql/operator/UpdateOperator.java

presto-main/src/main/java/io/prestosql/sql/analyzer/StatementAnalyzer.java

djsstarburst · 2020-11-19T14:47:30Z

Thanks for the review, @abhishekkhanna1! As I mentioned, it would be great if you could figure out a way to add even very simple concurrency tests for Hive ACID operations, since we have almost none right now.

presto-spi/src/main/java/io/prestosql/spi/StandardErrorCode.java

abhishekkhanna1 · 2020-11-19T22:19:35Z

I will look into the next steps for the Invalid method name: 'alter_table_req' issue as being discussed here: https://prestosql.slack.com/archives/CGB0QHWSW/p1605751292043600
Next I will review the other commit specific to Hive ACID Update
I will also start looking into ways to write tests for conflicting transactions and rollbacks etc

abhishekkhanna1 · 2021-02-02T02:17:33Z

Hi @djsstarburst Can we push these commits while we work on improving the limitations as part of #3325 separately?

This commit adds parsing, analysis and query planning for SQL UPDATE statements. A second commit on top of this commit adds Hive ACID support for UPDATE, and product tests that show UPDATE works as expected. This commit includes a new developer documentation page for connector developers, delete-and-update.rst, that explains in detail how DELETE and UPDATE work. This document refers to the Hive ACID implementation of UPDATE, coming in the second commit. One note for the reader: before this commit, method ConnectorMetadata method getUpdateRowIdColumnHandle() was used to get the rowId column handle for DELETE operations. This was renamed to getDeleteRowIdColumnHandle(), to make way for the new getUpdateRowIdColumnHandle(), used to get the rowId column handle for UPDATE operations.

This commit implements SQL UPDATE for Hive ACID Tables, and adds product tests that demonstrate that it works as expected. Those tests check: o Expected failures cases - - e.g., non-transactional table; updating partition or bucket columns. o Some or all columns of the table updated. o A spanning set of cases of overlap between dependency columns and updated columns. o A spanning set of cases of overlap between dependency columns and non-updated columns. o Permuted order of columns updated compared to table declaration order. o Unpartitioned, partitioned and bucketed table updates. o Some or all columns updated to either litereal or computed NULLs.

electrum · 2021-02-03T22:51:20Z

Thanks for the work on this @djsstarburst!

XiaoqiMa · 2021-03-01T11:45:15Z

@djsstarburst Hi, David. I have seen your commit. But do you have any demo code for us to learn how to implement UPDATE and DELETE for JDBC-based connectors. Thanks

djsstarburst · 2021-03-01T20:43:23Z

@djsstarburst Hi, David. I have seen your commit. But do you have any demo code for us to learn how to implement UPDATE and DELETE for JDBC-based connectors. Thanks

I don't have example code, but how UPDATE works is documented in detail in:
https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/develop/delete-and-update.rst

findepi · 2021-03-23T13:42:28Z

core/trino-spi/src/main/java/io/trino/spi/connector/UpdatablePageSource.java

+        throw new UnsupportedOperationException("This connector does not support row-level delete");
+    }
+
+    default void updateRows(Page page, List<Integer> columnValueAndRowIdChannels)


Add some info how this list should be decomposed, perhaps as a javadoc

djsstarburst · 2021-03-23T13:58:13Z

Add some info how this list should be decomposed, perhaps as a javadoc

Have you seen the developer documentation for DELETE and UPDATE?
https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/develop/delete-and-update.rst

findepi · 2021-03-23T14:56:33Z

Good point. We should either link to the full docs from the javadoc or add some excerpts from docs into a javadoc.
The good side is that you have good wording for everything already.

divyanshidm · 2023-02-06T11:48:06Z

Is this available in Trino? (I am using Trino CLI 376, Hive Metastore 3.1.2, Both on dataproc gcp.)
But it still complains on update/delete - "Invalid method name: 'alter_table_req'"
Can somebody help?

cla-bot bot added the cla-signed label Nov 6, 2020

djsstarburst requested review from martint, electrum, findepi and losipiuk November 6, 2020 15:20

djsstarburst mentioned this pull request Nov 6, 2020

UPDATE statement #3325

Open

8 tasks

martint reviewed Nov 6, 2020

View reviewed changes

djsstarburst force-pushed the david.stryker/hive-acid-update branch 2 times, most recently from 9713f42 to 4dd2668 Compare November 6, 2020 18:04

martint self-requested a review November 6, 2020 19:57

martint reviewed Nov 6, 2020

View reviewed changes

martint self-requested a review November 6, 2020 20:30

djsstarburst force-pushed the david.stryker/hive-acid-update branch from 4dd2668 to f776059 Compare November 7, 2020 01:16

djsstarburst force-pushed the david.stryker/hive-acid-update branch 4 times, most recently from c491f42 to 6bc9c60 Compare November 10, 2020 01:12

electrum reviewed Nov 19, 2020

View reviewed changes

djsstarburst force-pushed the david.stryker/hive-acid-update branch 2 times, most recently from 0cdedf5 to 25d4987 Compare November 19, 2020 04:32

djsstarburst force-pushed the david.stryker/hive-acid-update branch 2 times, most recently from e25c048 to 2bf6506 Compare November 19, 2020 05:48

abhishekkhanna1 reviewed Nov 19, 2020

View reviewed changes

djsstarburst force-pushed the david.stryker/hive-acid-update branch from 2bf6506 to 3c46bc1 Compare November 19, 2020 15:02

electrum reviewed Nov 19, 2020

View reviewed changes

presto-spi/src/main/java/io/prestosql/spi/StandardErrorCode.java Outdated Show resolved Hide resolved

djsstarburst force-pushed the david.stryker/hive-acid-update branch 8 times, most recently from 1f87ba4 to 70fe17c Compare January 8, 2021 22:40

djsstarburst mentioned this pull request Jan 30, 2021

Improve SQL UPDATE support #6775

Closed

3 tasks

djsstarburst force-pushed the david.stryker/hive-acid-update branch from 70fe17c to e68b6b2 Compare February 2, 2021 23:09

electrum approved these changes Feb 3, 2021

View reviewed changes

djsstarburst force-pushed the david.stryker/hive-acid-update branch 2 times, most recently from a11dd8d to 3fbdb8f Compare February 3, 2021 17:04

djsstarburst force-pushed the david.stryker/hive-acid-update branch from 3fbdb8f to 7d8e8ac Compare February 3, 2021 18:46

electrum merged commit cf68459 into trinodb:master Feb 3, 2021

djsstarburst deleted the david.stryker/hive-acid-update branch February 3, 2021 23:05

martint added this to the 352 milestone Feb 4, 2021

electrum mentioned this pull request Feb 9, 2021

Release notes for 352 #6502

Closed

10 tasks

djsstarburst mentioned this pull request Feb 24, 2021

Allow creation of and writing to Hive transactional/ACID tables in Presto #1956

Closed

findepi reviewed Mar 23, 2021

View reviewed changes

losipiuk mentioned this pull request Apr 16, 2021

Hive ORC ACID INSERT/UPDATE/DELETE/MERGE support #7611

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SQL UPDATE in the Presto Engine and Hive ACID Tables #5861

Support SQL UPDATE in the Presto Engine and Hive ACID Tables #5861

djsstarburst commented Nov 6, 2020

martint left a comment

djsstarburst commented Nov 7, 2020

abhishekkhanna1 commented Nov 12, 2020

electrum left a comment

djsstarburst commented Nov 19, 2020

abhishekkhanna1 Nov 19, 2020

djsstarburst Nov 19, 2020

electrum Nov 19, 2020

abhishekkhanna1 Nov 19, 2020

djsstarburst commented Nov 19, 2020

abhishekkhanna1 commented Nov 19, 2020

abhishekkhanna1 commented Feb 2, 2021

electrum commented Feb 3, 2021

XiaoqiMa commented Mar 1, 2021

djsstarburst commented Mar 1, 2021

findepi Mar 23, 2021

djsstarburst commented Mar 23, 2021

findepi commented Mar 23, 2021

divyanshidm commented Feb 6, 2023

Support SQL UPDATE in the Presto Engine and Hive ACID Tables #5861

Support SQL UPDATE in the Presto Engine and Hive ACID Tables #5861

Conversation

djsstarburst commented Nov 6, 2020

martint left a comment

Choose a reason for hiding this comment

djsstarburst commented Nov 7, 2020

abhishekkhanna1 commented Nov 12, 2020

electrum left a comment

Choose a reason for hiding this comment

djsstarburst commented Nov 19, 2020

abhishekkhanna1 Nov 19, 2020

Choose a reason for hiding this comment

djsstarburst Nov 19, 2020

Choose a reason for hiding this comment

electrum Nov 19, 2020

Choose a reason for hiding this comment

abhishekkhanna1 Nov 19, 2020

Choose a reason for hiding this comment

djsstarburst commented Nov 19, 2020

abhishekkhanna1 commented Nov 19, 2020

abhishekkhanna1 commented Feb 2, 2021

electrum commented Feb 3, 2021

XiaoqiMa commented Mar 1, 2021

djsstarburst commented Mar 1, 2021

findepi Mar 23, 2021

Choose a reason for hiding this comment

djsstarburst commented Mar 23, 2021

findepi commented Mar 23, 2021

divyanshidm commented Feb 6, 2023