Use force repartitioning for OPTIMIZE for iceberg #10619

homar · 2022-01-14T16:05:30Z

For all partitioned tables it makes sense to force repartitiong
while performing OPTIMIZE. Previously it was forced only if all fields
in partition spec had identinty transform.

electrum · 2022-01-14T22:23:27Z

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

@@ -481,7 +481,7 @@ public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, Con
                .map(IcebergColumnHandle::getName)
                .collect(toImmutableList());

-        if (partitionSpec.fields().stream().allMatch(field -> field.transform().isIdentity())) {
+        if (partitionSpec.fields().stream().allMatch(field -> field.transform().isIdentity()) && !forceRepartitioning) {


Flip && so the simple boolean comes first

electrum · 2022-01-14T22:25:31Z

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

            throws IOException
    {
        String schema = getSession().getSchema().orElseThrow();
        Path tableDataDir = getDistributedQueryRunner().getCoordinator().getBaseDataDir().resolve("iceberg_data").resolve(schema).resolve(tableName).resolve("data");
        try (Stream<Path> list = Files.list(tableDataDir)) {
            return list
+                    .flatMap(path -> {
+                        try {
+                            return Files.walk(path).filter(Files::isRegularFile);


Change Files.list() above to use Files.walk()

electrum · 2022-01-14T22:25:55Z

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

+                        try {
+                            return Files.walk(path).filter(Files::isRegularFile);
+                        }
+                        catch (IOException e) {


This should not occur. If it does, something is wrong and we should fail

yes I know but as IOException is a checked one and it was inside the flatMap I had to do something with it..

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

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

findepi · 2022-01-18T09:58:04Z

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

    {
        return computeActual(format("SELECT file_path FROM \"%s$files\"", tableName)).getOnlyColumn()
                .map(String.class::cast)
-                .collect(toImmutableList());
+                .collect(toImmutableSet());


i guess @findepi used List here intentionally. Why the change?

I changed it to have the same way as in delta lake but if it was intentional I will revert my change

findepi · 2022-01-18T10:02:13Z

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

-                    .map(Path::toString)
-                    .collect(toImmutableList());
-        }
+        return Files.walk(tableDataDir)


from Files.walk docs

This method must be used within a try-with-resources statement or similar control structure to ensure that the stream's open directories are closed promptly after the stream's operations have completed.

Anyway, the changes here look as not related to the main part of the commit, while they actually are.
I'd extract this to a prep commit like "Let getAllDataFilesFromTableDirectory recurse directories"

findepi · 2022-01-18T10:02:31Z

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

+            // optimize an empty table
+            assertQuerySucceeds("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
+            assertThat(getActiveFiles(tableName)).isEmpty();


findepi · 2022-01-18T10:03:45Z

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

+            throws IOException
+    {
+        String tableName = "test_repartitiong_during_optimize_" + randomTableSuffix();
+        assertUpdate("CREATE TABLE " + tableName + " (key integer, value varchar) WITH (partitioning = ARRAY['value'])");


Partitioning on "value" is not natural.
Also it's more natural for partitioning column to come first in the table schema: pk integer, value varchar) ...

findepi · 2022-01-18T10:05:15Z

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

@@ -3339,4 +3340,43 @@ public void testOptimizeParameterValidation()
                "ALTER TABLE nation EXECUTE OPTIMIZE (file_size_threshold => '33s')",
                "\\QUnable to set procedure property 'file_size_threshold' to ['33s']: Unknown unit: s");
    }
+
+    @Test
+    public void testRepartitioningIsForcedDuringOptimize()


We should have tests for OPTIMIZE with partitioned tables.
Actually, this may be the test. Just rename it.
The "forced repartitioning" will then be an inline comment where the assertion is done

findepi · 2022-01-18T10:07:13Z

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

+            Set<String> initialFiles = getActiveFiles(tableName);
+            assertThat(initialFiles).hasSize(10);
+
+            computeActual("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");


Use session with high preferred_write_partitioning_min_number_of_partitions to verify repartitioning is indeed forced, as the test class could have it artificially lowered.

findepi · 2022-01-18T10:08:04Z

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

+            assertThat(updatedFiles).hasSize(3);
+            assertThat(getAllDataFilesFromTableDirectory(tableName)).isEqualTo(union(initialFiles, updatedFiles));
+        }
+        finally {


the try finally is not needed here, as the test runs on isolated resources (a temp folder).
you can omit it for improved readability

homar · 2022-01-19T10:16:36Z

@findepi do you mind taking another look ?

homar · 2022-01-19T14:41:22Z

One unrelated failure, I created an issue for it #10693

findepi · 2022-01-20T11:10:32Z

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

+    {
+        // This test will have its own session to make sure partitioning is indeed forced and is not a result
+        // of session configuration
+        Session currentSession = testSessionBuilder()


call it session

findepi · 2022-01-20T11:10:57Z

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

+        return getActiveFiles(tableName, getQueryRunner().getDefaultSession());
+    }
+
+    private List<String> getActiveFiles(String tableName, Session session)


getActiveFiles result doesn't depend on the session; the fact it uses SELECT should be viewed as impl detail, it could go directly to storage, or use Iceberg APIs directly instead

no need to pass session arg here.

findepi · 2022-01-20T11:12:44Z

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

+                .setSystemProperty("use_preferred_write_partitioning", "false")
+                .setSystemProperty("preferred_write_partitioning_min_number_of_partitions", "100")


use_preferred_write_partitioning=false cancels preferred_write_partitioning_min_number_of_partitions
i think use_preferred_write_partitioning should be true

For all partitioned tables it makes sense to force repartitiong while performing OPTIMIZE. Previously it was forced only if all fields in partition spec had identinty transform.

cla-bot bot added the cla-signed label Jan 14, 2022

electrum reviewed Jan 14, 2022

View reviewed changes

homar force-pushed the homar/force_repartitiong_for_iceberg_optimize branch from 8171387 to 22bb2ee Compare January 15, 2022 11:34

homar requested a review from findepi January 18, 2022 09:34

findepi reviewed Jan 18, 2022

View reviewed changes

homar added 2 commits January 19, 2022 10:55

Fix indentation in BaseIcebergConnectorTest

e04bd4d

Let getAllDataFilesFromTableDirectory recurse directories

ae5c8a0

homar force-pushed the homar/force_repartitiong_for_iceberg_optimize branch from 22bb2ee to c7b9667 Compare January 19, 2022 10:16

homar force-pushed the homar/force_repartitiong_for_iceberg_optimize branch from c7b9667 to 645a418 Compare January 19, 2022 11:32

findepi approved these changes Jan 20, 2022

View reviewed changes

Use force repartitioning for OPTIMIZE for iceberg

01b02cf

For all partitioned tables it makes sense to force repartitiong while performing OPTIMIZE. Previously it was forced only if all fields in partition spec had identinty transform.

homar force-pushed the homar/force_repartitiong_for_iceberg_optimize branch from 645a418 to 01b02cf Compare January 20, 2022 11:46

findepi merged commit affdeb9 into trinodb:master Jan 21, 2022

github-actions bot added this to the 369 milestone Jan 21, 2022

This was referenced Jan 21, 2022

Add Trino 369 release notes #10553

Merged

Release notes for 369 #10552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use force repartitioning for OPTIMIZE for iceberg #10619

Use force repartitioning for OPTIMIZE for iceberg #10619

homar commented Jan 14, 2022

electrum Jan 14, 2022

electrum Jan 14, 2022

electrum Jan 14, 2022

homar Jan 15, 2022

findepi Jan 18, 2022

homar Jan 19, 2022

findepi Jan 18, 2022

homar Jan 19, 2022

findepi Jan 18, 2022

findepi Jan 18, 2022

findepi Jan 18, 2022

findepi Jan 18, 2022

findepi Jan 18, 2022

homar commented Jan 19, 2022

homar commented Jan 19, 2022

findepi Jan 20, 2022

findepi Jan 20, 2022

homar Jan 20, 2022

findepi Jan 20, 2022

		.setSystemProperty("use_preferred_write_partitioning", "false")
		.setSystemProperty("preferred_write_partitioning_min_number_of_partitions", "100")

Use force repartitioning for OPTIMIZE for iceberg #10619

Use force repartitioning for OPTIMIZE for iceberg #10619

Conversation

homar commented Jan 14, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Jan 19, 2022

homar commented Jan 19, 2022

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