Fix delta lake CDF entries for MERGE queries #16127

homar · 2023-02-15T15:09:22Z

Description

Before this change MERGE that made both UPDATE and DELETE could create CDF entries with rows marked as delete when in fact there were update_preimage and other way around

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Fix change data feed entries for `MERGE` queries. ({issue}`16127`)

alexjo2144

Good catch

alexjo2144 · 2023-02-15T15:46:44Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java

        LongBitmapDataProvider rowsToDelete = deletion.rowsToDelete;
+        LongBitmapDataProvider rowsDeletedBecauseOfUpdate = deletion.rowsDeletedBecauseOfUpdate();


Can we avoid marking update preimage rows in both Bitmaps?

i don't think so, we need to carry information if particular position deletion came from DELETE or from UPDATE, instead of a bitmap a normal Map could be used that would store the reason for the deletion but I assumed it is more efficient to use 2 bitmaps.
I can change to one Map if you think it is better

Two Bitmaps is good, but you have redundant information for updated rows, the positions for both rowsToDelete and rowsDeletedBecauseOfUpdate are set. I'd expect you to set one or the other, not both.

ok I think I figured out your point, and now the second if would probably also work the way you wrote it, I will give it a shot. thanks

alexjo2144 · 2023-02-15T15:48:10Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java

                int retainedCount = 0;
                int deletedCount = 0;
+                int deletedBecauseOfUpdateCount = 0;
+                int deletedBecauseOfDeleteCount = 0;
                for (int position = 0; position < positionCount; position++) {
                    if (!rowsToDelete.contains(filePosition)) {


I'd rephrase these as:

if (rowsToDelete.contains(filePosition)) { ... } else if (rowsDeletedBecauseOfUpdate.contains(filePosition)) { ... } else { // retain ... }

But that would be incorrect. I am interested in rows that will be deleted I just need to split them into 2 types

Ok my bad, I figured it out and it works. Thanks

findinpath · 2023-02-16T05:56:25Z

Build is red (error-prone-checks)

 Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.9.0:compile (default-compile) on project trino-delta-lake: Compilation failure: Compilation failure: 
Error:  /home/runner/work/trino/trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java:[418,23] [UnusedVariable] The local variable 'deleted' is never read.

findinpath · 2023-02-16T05:58:58Z

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java

+            onDelta().executeQuery("INSERT INTO default." + tableName2 + " VALUES (4, 'nation4000', 4000)");
+
+            onTrino().executeQuery("MERGE INTO delta.default." + tableName1 + " cdf USING delta.default." + tableName2 + " n " +
+                    "ON (cdf.nationkey = n.nationkey) " +


nit: Use text blocks

hmm ok but I am not a big fun, the fact they don't support interpolation is a no go for me

#16127 (comment) we will stick to strings, sorry

findinpath · 2023-02-16T06:16:47Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java

@@ -410,21 +416,30 @@ private Optional<DataFileInfo> rewriteParquetFile(Path path, FileDeletion deleti
                int positionCount = page.getPositionCount();
                int[] retained = new int[positionCount];
                int[] deleted = new int[positionCount];
+                int[] deletedBecauseOfUpdate = new int[positionCount];


We created before this change one array with positionCount items. Now we create two arrays with positionCount items. Is this valid?

We created 2 arrays with positionCount:

int[] retained = new int[positionCount]; int[] deleted = new int[positionCount];

now we create three, this is valid, these arrays are only used to get specific positions from page

Why not use rowsDeletedBecauseOfUpdate.getLongCardinality() instead?

findinpath · 2023-02-16T06:18:23Z

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java

+                    "USING DELTA " +
+                    "LOCATION 's3://" + bucketName + "/databricks-compatibility-test-" + tableName1 + "'" +
+                    "TBLPROPERTIES (delta.enableChangeDataFeed = true)");
+            onDelta().executeQuery("CREATE TABLE default." + tableName2 + " (nationkey INT, name STRING, regionkey INT) " +


this table does not need to enable cdf

findinpath · 2023-02-16T06:19:25Z

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java

+    @Test(groups = {DELTA_LAKE_DATABRICKS, DELTA_LAKE_EXCLUDE_73, PROFILE_SPECIFIC_TESTS})
+    public void testMergeMixedDeleteAndUpdateIntoTableWithCdfEnabled()
+    {
+        String tableName1 = "test_merge_mixed_delete_and_update_into_table_with_cdf_" + randomNameSuffix();


tableName1 -> targetTableName
tableName2 -> sourceTableName

as in the syntax of MERGE https://trino.io/docs/current/sql/merge.html

findinpath · 2023-02-16T06:23:53Z

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java

+                    "WHEN MATCHED AND cdf.nationkey = 2 " +
+                    "THEN DELETE " +
+                    "WHEN MATCHED AND cdf.nationkey > 2 " +
+                    "THEN UPDATE SET nationkey = (cdf.nationkey + n.nationkey + n.regionkey) " +


optional nit for getting the test easier to read

Let's rather update the field you have now named as regionkey to easier follow what gets changes by having the nationkey as key.

I'd go with a page_views (pagekey, url, views) sample which is easier to follow (when merging a page with the same id, the views get summed).

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java

homar · 2023-02-16T17:12:09Z

@ebyhr could you please run this one with secrets ?

ebyhr · 2023-02-17T05:12:20Z

/test-with-secrets sha=f1c89aadc1ab6472d4c292a04e55daa2c8a511bb

ebyhr · 2023-02-17T09:03:50Z

CI is running in https://github.com/trinodb/trino/actions/runs/4201339088/jobs/7288250233

github-actions · 2023-02-17T09:08:44Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4201339088

Before this change MERGE that made both UPDATE and DELETE could create CDF entries with rows marked as delete when in fact there were update_preimage and other way around

homar requested a review from findinpath February 15, 2023 15:09

cla-bot bot added the cla-signed label Feb 15, 2023

homar requested review from alexjo2144 and ebyhr February 15, 2023 15:09

alexjo2144 reviewed Feb 15, 2023

View reviewed changes

alexjo2144 requested a review from djsstarburst February 15, 2023 16:02

github-actions bot added the tests:hive label Feb 15, 2023

homar force-pushed the homar/fix_cdf_entries_for_merge_with_update_and_delete branch from 881b5b2 to 1b534fb Compare February 15, 2023 19:37

homar requested a review from alexjo2144 February 15, 2023 19:38

findinpath reviewed Feb 16, 2023

View reviewed changes

homar requested a review from findinpath February 16, 2023 09:14

homar force-pushed the homar/fix_cdf_entries_for_merge_with_update_and_delete branch from 1b534fb to 982fc2d Compare February 16, 2023 09:14

ebyhr reviewed Feb 16, 2023

View reviewed changes

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java Outdated Show resolved Hide resolved

...ava/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksChangeDataFeedCompatibility.java Outdated Show resolved Hide resolved

homar force-pushed the homar/fix_cdf_entries_for_merge_with_update_and_delete branch from 982fc2d to f1c89aa Compare February 16, 2023 13:56

homar requested a review from ebyhr February 16, 2023 13:56

findinpath approved these changes Feb 17, 2023

View reviewed changes

Fix delta lake CDF entries for MERGE queries

ea53061

Before this change MERGE that made both UPDATE and DELETE could create CDF entries with rows marked as delete when in fact there were update_preimage and other way around

ebyhr force-pushed the homar/fix_cdf_entries_for_merge_with_update_and_delete branch from f1c89aa to ea53061 Compare February 17, 2023 09:20

ebyhr merged commit 289dd57 into trinodb:master Feb 17, 2023

ebyhr mentioned this pull request Feb 17, 2023

Release notes for 408 #16147

Closed

github-actions bot added this to the 408 milestone Feb 17, 2023

colebow mentioned this pull request Feb 21, 2023

Add Trino 408 release notes #16209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delta lake CDF entries for MERGE queries #16127

Fix delta lake CDF entries for MERGE queries #16127

homar commented Feb 15, 2023 •

edited by ebyhr

Loading

alexjo2144 left a comment

alexjo2144 Feb 15, 2023

homar Feb 15, 2023

alexjo2144 Feb 15, 2023

homar Feb 15, 2023

alexjo2144 Feb 15, 2023

homar Feb 15, 2023

homar Feb 15, 2023

findinpath commented Feb 16, 2023

findinpath Feb 16, 2023

homar Feb 16, 2023

homar Feb 16, 2023

findinpath Feb 16, 2023

homar Feb 16, 2023

findinpath Feb 16, 2023

findinpath Feb 16, 2023

findinpath Feb 16, 2023

findinpath Feb 16, 2023

homar commented Feb 16, 2023

ebyhr commented Feb 17, 2023

ebyhr commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

		LongBitmapDataProvider rowsToDelete = deletion.rowsToDelete;
		LongBitmapDataProvider rowsDeletedBecauseOfUpdate = deletion.rowsDeletedBecauseOfUpdate();

Fix delta lake CDF entries for MERGE queries #16127

Fix delta lake CDF entries for MERGE queries #16127

Conversation

homar commented Feb 15, 2023 • edited by ebyhr Loading

Description

Additional context and related issues

Release notes

alexjo2144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Feb 16, 2023

ebyhr commented Feb 17, 2023

ebyhr commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

homar commented Feb 15, 2023 •

edited by ebyhr

Loading