Do not use metadata deletes during Iceberg writes #18533

alexjo2144 · 2023-08-03T20:08:29Z

Description

This is an alternative to #18412 but with more minimal changes.

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Iceberg
* Fix write conflict detection for `UPDATE/DELETE/MERGE` operations. In rare situations this issue may have resulted in duplicate rows when multiple of these operations were run at the same time, or at the same time as an `optimize` procedure.

findepi · 2023-08-04T12:48:23Z

some tests have failed

ksobolew · 2023-08-04T14:52:59Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergStatistics.java

@@ -154,8 +154,8 @@ public void testAnalyzeWithSchemaEvolution()
                        VALUES
                          ('nationkey', null, 25, 0, null, '0', '24'),
                          ('regionkey', null, 5, 0, null, '0', '4'),
-                          ('name', 1314.0, 25, 0, null, null, null),
-                          ('info', 4417.0, null, 0, null, null, null),
+                          ('name', 1907.9999999999998, 25, 0, null, null, null),


That does not look like reliably repeatable result

Yeah, it's also different when I run the test locally.

does it mean the test doesn't pass locally, or is flaky?

BTW double results are asserted with rounding. you should probably have 1908.0 instead of 1907.9999999999998
(locally the test passed for me with both values; i would use the shorter one)

does it mean the test doesn't pass locally, or is flaky?

Consistently fails. We chatted about this test on Slack the other day

alexjo2144 · 2023-08-07T13:32:42Z

@findepi last build was green, just squashed

findepi · 2023-08-07T13:36:22Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergStatistics.java

@@ -154,8 +154,8 @@ public void testAnalyzeWithSchemaEvolution()
                        VALUES
                          ('nationkey', null, 25, 0, null, '0', '24'),
                          ('regionkey', null, 5, 0, null, '0', '4'),
-                          ('name', 1314.0, 25, 0, null, null, null),
-                          ('info', 4417.0, null, 0, null, null, null),
+                          ('name', 1907.9999999999998, 25, 0, null, null, null),


does it mean the test doesn't pass locally, or is flaky?

findepi · 2023-08-07T13:39:21Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergStatistics.java

@@ -154,8 +154,8 @@ public void testAnalyzeWithSchemaEvolution()
                        VALUES
                          ('nationkey', null, 25, 0, null, '0', '24'),
                          ('regionkey', null, 5, 0, null, '0', '4'),
-                          ('name', 1314.0, 25, 0, null, null, null),
-                          ('info', 4417.0, null, 0, null, null, null),
+                          ('name', 1907.9999999999998, 25, 0, null, null, null),


BTW double results are asserted with rounding. you should probably have 1908.0 instead of 1907.9999999999998
(locally the test passed for me with both values; i would use the shorter one)

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergStatistics.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java

findepi · 2023-08-07T13:48:55Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

@@ -124,28 +128,38 @@ public void testDeleteRowsConcurrently()
        int threads = 4;
        CyclicBarrier barrier = new CyclicBarrier(threads);
        ExecutorService executor = newFixedThreadPool(threads);
+        List<String> rows = ImmutableList.of("(1, 0, 0, 0)", "(0, 1, 0, 0)", "(0, 0, 1, 0)", "(0, 0, 0, 1)");
+
+        String[] expectedErrors = new String[]{"Failed to commit Iceberg update to table:", "Failed to replace table due to concurrent updates:"};


verifyConcurrentUpdateFailurePermissible allows "Failed to commit Iceberg update to table:" only. is the latter possible as well?

findepi · 2023-08-07T13:50:24Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

-            assertUpdate("INSERT INTO " + tableName + " VALUES (1, 0, 0, 0)", 1);
-            assertUpdate("INSERT INTO " + tableName + " VALUES (0, 1, 0, 0)", 1);
-            assertUpdate("INSERT INTO " + tableName + " VALUES (0, 0, 1, 0)", 1);
-            assertUpdate("INSERT INTO " + tableName + " VALUES (0, 0, 0, 1)", 1);


The test created 5 files, now it creates ~1 file with 4 rows.
If this doesn't matter for the test purpose, let's do this change as a separate commit.
if this matters and should always be so (~1 file), let's do this as a separate (prep) commit.

It used to matter but doesn't any more. One line per file guaranteed that we always used metadata deletes in this test but now we no longer use those.

findepi · 2023-08-07T13:52:28Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

@@ -1991,6 +1992,7 @@ public void testPartitionPredicatePushdownWithHistoricalPartitionSpecs()

        // The old partition scheme is no longer used so pushdown using the hour transform is allowed
        assertUpdate("DELETE FROM " + tableName + " WHERE year(d) = 1969", 3);
+        assertUpdate("ALTER TABLE " + tableName + " EXECUTE optimize");


why change this test?
(separate commit?)

The test relied on the behavior that files were fully removed from the manifest during the delete operation, but now they are not until optimize is run.

findepi · 2023-08-07T13:52:35Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

@@ -1378,6 +1378,7 @@ public void testUpdateWithSortOrder()
                "test_sorted_update",
                "WITH (sorted_by = ARRAY['comment']) AS TABLE tpch.tiny.lineitem WITH NO DATA")) {
            assertUpdate(
+                    withSmallRowGroups,


why change this test?
(separate commit?)

The data created by the insert used to not be checked for if it was sorted because those files were removed from the manifest. Now they are checked for sorting, along with the files added by the update operations.

findepi · 2023-08-07T13:53:38Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

-        transaction = null;
-    }
-
-    private static boolean fileIsFullyDeleted(List<CommitTaskData> positionDeletes)


😢 i will miss you

Maybe it will rise from the ashes again...

The DeleteFiles operation does not validate conflicts like RowDelta does. In the event a file is removed during a write operation this can result in a update operation being reflected as an insert. As follow up this optimization could be put back using the OverwriteFiles API.

Code cleanup in IcebergMetadata

3d5d2e2

cla-bot bot added the cla-signed label Aug 3, 2023

alexjo2144 mentioned this pull request Aug 3, 2023

Fix Iceberg conflict verification during write operations #18412

Closed

alexjo2144 requested review from findepi and ebyhr August 3, 2023 20:09

alexjo2144 force-pushed the iceberg/file-deletion-v2 branch from 78528c3 to d38f2b7 Compare August 3, 2023 20:24

github-actions bot added the iceberg Iceberg connector label Aug 3, 2023

alexjo2144 force-pushed the iceberg/file-deletion-v2 branch 2 times, most recently from 4cd45f7 to b45178b Compare August 4, 2023 00:23

findepi approved these changes Aug 4, 2023

View reviewed changes

ksobolew reviewed Aug 4, 2023

View reviewed changes

alexjo2144 force-pushed the iceberg/file-deletion-v2 branch from c6df7b5 to 4829f2b Compare August 7, 2023 13:32

findepi approved these changes Aug 7, 2023

View reviewed changes

Test Iceberg statistics when a data file is fully deleted

846d8b7

alexjo2144 force-pushed the iceberg/file-deletion-v2 branch from 4829f2b to 437c1a5 Compare August 7, 2023 19:30

alexjo2144 force-pushed the iceberg/file-deletion-v2 branch from 437c1a5 to 71040d4 Compare August 8, 2023 14:22

findepi merged commit c6dfe27 into trinodb:master Aug 9, 2023

github-actions bot added this to the 423 milestone Aug 9, 2023

mosabua mentioned this pull request Aug 10, 2023

Add Trino 423 release notes #18496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use metadata deletes during Iceberg writes #18533

Do not use metadata deletes during Iceberg writes #18533

alexjo2144 commented Aug 3, 2023 •

edited

Loading

findepi commented Aug 4, 2023

ksobolew Aug 4, 2023

alexjo2144 Aug 4, 2023

findepi Aug 7, 2023

findepi Aug 7, 2023

alexjo2144 Aug 7, 2023

alexjo2144 commented Aug 7, 2023

findepi Aug 7, 2023

findepi Aug 7, 2023

findepi Aug 7, 2023

findepi Aug 7, 2023

alexjo2144 Aug 7, 2023

findepi Aug 7, 2023

alexjo2144 Aug 7, 2023

findepi Aug 7, 2023

alexjo2144 Aug 7, 2023

findepi Aug 7, 2023

alexjo2144 Aug 7, 2023

Do not use metadata deletes during Iceberg writes #18533

Do not use metadata deletes during Iceberg writes #18533

Conversation

alexjo2144 commented Aug 3, 2023 • edited Loading

Description

Additional context and related issues

Release notes

findepi commented Aug 4, 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

alexjo2144 commented Aug 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjo2144 commented Aug 3, 2023 •

edited

Loading