Support SQL MERGE in the Trino engine and five connectors #7933

djsstarburst · 2021-05-16T19:21:33Z

This PR is a second take on implementing SQL MERGE. It consists commits that add support for SQL MERGE in the Trino engine and in the Hive, Kudu, Raptor, Iceberg and Delta Lake connectors. The implementation is structured so that most of the work happens in the Trino engine, so adding support in a connector is pretty simple.

The SQL MERGE implementation allows update of all columns, including partition or bucket columns, and the Trino engine performs redistribution to ensure that the updated rows end up on the appropriate nodes.

The Trino engine commit introduces an enum RowChangeParadigm, which characterizes how a connector modifies rows. Hive uses and Iceberg will use the DELETE_ROW_AND_INSERT_ROW paradigm, since they represent an updated row as a deleted row and an inserted row. Kudu uses the CHANGE_ONLY_UPDATED_COLUMNS paradigm.

Each paradigm corresponds to an implementation of the RowChangeProcessor interface. After this PR is merged, the intent is to retrofit SQL UPDATE to use the same RowChangeParadigm/Processor mechanism.

Extensive documentation on the internal MERGE architecture can be found in the developer doc supporting-merge.rst.

Fixes #7708

docs/src/main/sphinx/develop/supporting-merge.rst

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveWriterFactory.java

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/MergeFileWriter.java

electrum

The Kudu commit looks good

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

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

core/trino-main/src/main/java/io/trino/operator/DeleteAndInsertMergeProcessor.java

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java

djsstarburst · 2021-05-26T00:13:05Z

Thanks for the great comments, @electrum. I did everything you suggested.

kasiafi

A lot of questions and some comments. I've gone through the docs, and partially through the analysis.

docs/src/main/sphinx/develop/supporting-merge.rst

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

core/trino-main/src/main/java/io/trino/sql/analyzer/CanonicalizationAware.java

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

kasiafi

Some more comments regarding the analyzer. Initial comments on the planner part.

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java

djsstarburst · 2021-06-16T18:39:43Z

Thanks for the great first batch of comments, @kasiafi! I believe I've addressed the comments from yesterday except those listed below. It would be great if you could resolve the comments you think have been handled to your satisfaction.

I haven't addressed the more profound comments made 4 hours ago yet, and some of them will require coaching from you or @martint.

Here are the comments from yesterday that I haven't addressed:

Does DuplicateRowFinder need to compare the writeRedistribution columns?
Will matched target table rowIds really come out in order such that DuplicateRowFinder is guaranteed to identify them?
Implementing multiple assignment.
Addressing your comment: "Instead of assigning a scope to an Identifier, the aliased table should parse as AliasedRelation."
Addressing your comment: "What if the table was a materialized view?"

findepi · 2021-06-17T07:36:14Z

re #7933 (comment)

target table rowIds would be partitioned among nodes

@djsstarburst can you please point me to a document outlining how MERGE interacts with connectors?

i would like to learn about the following

what are the assumption on rowIds, can rowIds carry un-updated columns
how should a connector construct rowIds if it needs to create deletion delta files for the sake of updates (e.g. a separate deletion file for an input file which would mark all the rows that got updated)
what is table handle lifecycle for MERGE. for example, how MERGE interacts with partition, file and file chunk pruning

docs/src/main/sphinx/develop/supporting-merge.rst

docs/src/main/sphinx/sql/merge.rst

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java

kasiafi

Here are some comments regarding the previously reviewed part. Additionally, I answered some of your replies directly. I resolved all conversations except those that require a follow-up.

I plan to review next portions of code, and put my comments in a new batch.

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

core/trino-main/src/main/java/io/trino/operator/DeleteAndInsertMergeProcessor.java

findepi · 2021-06-17T11:02:25Z

core/trino-main/src/main/java/io/trino/operator/DeleteAndInsertMergeProcessor.java

+            if (underlyingBlock instanceof RowBlock) {
+                List<Block> newRowIdChildrenBuilder = new ArrayList<>();
+                rowIdBlock.getChildren().stream()
+                        .map(block -> block.getPositions(rowIdPositions, 0, totalPositions))
+                        .forEach(newRowIdChildrenBuilder::add);
+                return RowBlock.fromFieldBlocks(
+                        totalPositions,
+                        Optional.empty(),
+                        newRowIdChildrenBuilder.toArray(new Block[] {}));
+            }
+            else {
+                return rowIdBlock.getPositions(rowIdPositions, 0, totalPositions);


Why RowBlock is special-cased here?
What if underlyingBlock is a DictionaryBlock over a RowBlock? Would it require special-casing as well?

I had endless trouble with this, and it's one of the main things I hoped review would shed light on.

I had hoped that I could just call rowIdBlock.getPositions(...) and end up with a consistent view of the resulting block. However, when I tried that, way downstream in the Driver I would see out-of-range array references. My assumption is that I'm doing something wrong, but I wasn't successful debugging the problem.

I had endless trouble with this, and it's one of the main things I hoped review would shed light on.

Sorry that i cannot help. Add a TODO comment here, warning the reader we don't exactly know why it's written the way it's written

findepi · 2021-06-17T11:05:39Z

core/trino-main/src/main/java/io/trino/operator/DeleteAndInsertMergeProcessor.java

+        Arrays.fill(nulls, true);
+        if (underlyingBlock instanceof RowBlock) {
+            return RowBlock.fromFieldBlocks(positionCount, Optional.of(nulls), rowIdBlock.getChildren().toArray(new Block[]{}));
+        }
+        else {
+            return ArrayBlock.fromElementBlock(positionCount, Optional.of(nulls), new int[positionCount], underlyingBlock);
+        }


Shouldn't this actually depend on rowIdType?

also, direct use of ArrayBlock is not correct. Typically you would use io.trino.spi.type.Type#createBlockBuilder(io.trino.spi.block.BlockBuilderStatus, int) to construct a block of values for given type.

Here, however, you actually want to create a single-value NULL block (nativeValueToBlock may be helpful) and wrap it in a RunLengthEncodedBlock instead

core/trino-main/src/main/java/io/trino/operator/DeleteAndInsertMergeProcessor.java

kasiafi

Some comments and questions regarding the planner part. I still have a few classes to review.

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PruneMergeSourceColumns.java

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/UnaliasSymbolReferences.java

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/SymbolMapper.java

docs/src/main/sphinx/sql/merge.rst

docs/src/main/sphinx/develop/supporting-merge.rst

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

...ino-blackhole/src/main/java/io/trino/plugin/blackhole/BlackHoleNodePartitioningProvider.java

This version works under emulation on M1 Macs.

This allows the engine to make the decision about how many nodes to use as appropriate, based on the number of workers or hash partition count session property. This is also required for MERGE so that the insert and update layouts can use the same mapping.

This commit adds support for SQL MERGE in the Trino engine. It introduces an enum RowChangeParadigm, which characterizes how a connector modifies rows. Hive and Iceberg will use the DELETE_ROW_AND_INSERT_ROW paradigm, since they represent an updated row as a deleted row and an inserted row. Kudu will use the CHANGE_ONLY_UPDATED_COLUMNS paradigm. Each paradigm corresponds to an implementation of the RowChangeProcessor interface. The intent is to retrofit SQL UPDATE to use the same RowChangeParadigm/Processor mechanism. The SQL MERGE implementation allows update of all columns, including partition or bucket columns, and the Trino engine performs redistribution to ensure that the updated rows end up on the appropriate nodes. MERGE processing is extensively documented in the new file in the developer documentation, supporting-merge.rst.

This commit adds SQL MERGE support in the Hive connector and a raft of MERGE tests to verify that it works.

cla-bot bot added the cla-signed label May 16, 2021

djsstarburst requested review from electrum, findepi, martint, kasiafi and dain May 16, 2021 19:21

djsstarburst mentioned this pull request May 16, 2021

Support SQL MERGE in the Trino engine and Hive and Kudu connectors #7386

Closed

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch from b87802f to d65286e Compare May 18, 2021 12:35

electrum reviewed May 25, 2021

View reviewed changes

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch from d65286e to f88718f Compare May 26, 2021 00:07

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch 2 times, most recently from 3108a8d to db83bfe Compare May 27, 2021 13:27

kasiafi reviewed Jun 15, 2021

View reviewed changes

kasiafi reviewed Jun 16, 2021

View reviewed changes

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch 2 times, most recently from 1b878ef to 238eb2d Compare June 16, 2021 17:31

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch 2 times, most recently from 6038c7f to b373e2b Compare June 16, 2021 19:03

kasiafi reviewed Jun 17, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java Outdated Show resolved Hide resolved

kasiafi reviewed Jun 17, 2021

View reviewed changes

findepi reviewed Jun 17, 2021

View reviewed changes

kasiafi reviewed Jun 17, 2021

View reviewed changes

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch 2 times, most recently from f4a18f7 to 083ab11 Compare June 17, 2021 15:10

findepi reviewed Jun 17, 2021

View reviewed changes

djsstarburst force-pushed the david.stryker/support-sql-merge-final branch from fb91326 to 541f751 Compare August 2, 2022 23:14

electrum force-pushed the david.stryker/support-sql-merge-final branch 3 times, most recently from b6beecb to 16f4b38 Compare August 3, 2022 21:15

dain approved these changes Aug 3, 2022

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorNodePartitioningProvider.java Show resolved Hide resolved

...ino-blackhole/src/main/java/io/trino/plugin/blackhole/BlackHoleNodePartitioningProvider.java Show resolved Hide resolved

electrum force-pushed the david.stryker/support-sql-merge-final branch 5 times, most recently from 361e835 to 3a2089a Compare August 4, 2022 03:39

electrum and others added 10 commits August 4, 2022 14:47

Update Kudu Toxiproxy to 2.1.4

5689dfb

This version works under emulation on M1 Macs.

Allow non-standard schemas for UPDATE smoke tests

1516b6d

Add support for TINYINT and REAL types in Raptor

0912cbd

Add CatalogHandle utility method in NodePartitioningManager

660ceb8

Add default implementation of getSplitBucketFunction

6b4c901

Support SQL MERGE in the Hive connector

e906548

This commit adds SQL MERGE support in the Hive connector and a raft of MERGE tests to verify that it works.

Support SQL MERGE in the Kudu connector

cf5c25c

Support SQL MERGE in the Raptor connector

435d100

electrum force-pushed the david.stryker/support-sql-merge-final branch from 3a2089a to 1d2fabd Compare August 4, 2022 21:47

electrum added 2 commits August 4, 2022 18:10

Support SQL MERGE in the Iceberg connector

6cb188b

Support SQL MERGE in the Delta Lake connector

53a4500

electrum force-pushed the david.stryker/support-sql-merge-final branch from 1d2fabd to 53a4500 Compare August 5, 2022 02:20

electrum merged commit 02fabc7 into trinodb:master Aug 5, 2022

github-actions bot added this to the 393 milestone Aug 5, 2022

nineinchnick mentioned this pull request Aug 8, 2022

Upgrade errorprone to version 2.15.0 #13540

Merged

jhlodin mentioned this pull request Aug 8, 2022

Add MERGE to SQL support documentation #13548

Merged

colebow mentioned this pull request Aug 8, 2022

Add Trino 393 release notes #13519

Merged

findepi mentioned this pull request Sep 6, 2022

Fix "No bucket node map" failure when inserting into Iceberg table #14003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SQL MERGE in the Trino engine and five connectors #7933

Support SQL MERGE in the Trino engine and five connectors #7933

djsstarburst commented May 16, 2021 •

edited by findepi

Loading

electrum left a comment

djsstarburst commented May 26, 2021 •

edited

Loading

kasiafi left a comment

kasiafi left a comment

djsstarburst commented Jun 16, 2021 •

edited

Loading

findepi commented Jun 17, 2021

kasiafi left a comment

findepi Jun 17, 2021

djsstarburst Jun 17, 2021

findepi Jun 18, 2021

findepi Jun 17, 2021

kasiafi left a comment

Support SQL MERGE in the Trino engine and five connectors #7933

Support SQL MERGE in the Trino engine and five connectors #7933

Conversation

djsstarburst commented May 16, 2021 • edited by findepi Loading

electrum left a comment

Choose a reason for hiding this comment

djsstarburst commented May 26, 2021 • edited Loading

kasiafi left a comment

Choose a reason for hiding this comment

kasiafi left a comment

Choose a reason for hiding this comment

djsstarburst commented Jun 16, 2021 • edited Loading

findepi commented Jun 17, 2021

kasiafi left a comment

Choose a reason for hiding this comment

findepi Jun 17, 2021

Choose a reason for hiding this comment

djsstarburst Jun 17, 2021

Choose a reason for hiding this comment

findepi Jun 18, 2021

Choose a reason for hiding this comment

findepi Jun 17, 2021

Choose a reason for hiding this comment

kasiafi left a comment

Choose a reason for hiding this comment

djsstarburst commented May 16, 2021 •

edited by findepi

Loading

djsstarburst commented May 26, 2021 •

edited

Loading

djsstarburst commented Jun 16, 2021 •

edited

Loading