From da181bfe63582fd379025714f5cdacce293977d5 Mon Sep 17 00:00:00 2001 From: Ping Liu Date: Tue, 5 Aug 2025 22:54:54 +0100 Subject: [PATCH] Fix rename column failed if the column is used as source column ofnon-identity transform --- .../iceberg/IcebergAbstractMetadata.java | 10 +- .../presto/iceberg/PartitionFields.java | 10 ++ .../iceberg/IcebergDistributedTestBase.java | 96 ++++++++++++++++++- 3 files changed, 111 insertions(+), 5 deletions(-) diff --git a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java index e6558265aabb5..d40e6e6d174f3 100644 --- a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java +++ b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java @@ -83,7 +83,6 @@ import org.apache.iceberg.IsolationLevel; import org.apache.iceberg.MetricsConfig; import org.apache.iceberg.MetricsModes.None; -import org.apache.iceberg.PartitionField; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.RowDelta; import org.apache.iceberg.RowLevelOperationMode; @@ -904,9 +903,12 @@ public void renameColumn(ConnectorSession session, ConnectorTableHandle tableHan Table icebergTable = getIcebergTable(session, icebergTableHandle.getSchemaTableName()); Transaction transaction = icebergTable.newTransaction(); transaction.updateSchema().renameColumn(columnHandle.getName(), target).commit(); - if (icebergTable.spec().fields().stream().map(PartitionField::sourceId).anyMatch(sourceId -> sourceId == columnHandle.getId())) { - transaction.updateSpec().renameField(columnHandle.getName(), target).commit(); - } + icebergTable.spec().fields().stream() + .filter(field -> field.sourceId() == columnHandle.getId()) + .forEach(field -> { + String transform = field.transform().toString(); + transaction.updateSpec().renameField(field.name(), getPartitionColumnName(target, transform)).commit(); + }); transaction.commitTransaction(); } diff --git a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionFields.java b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionFields.java index 7668efea0f791..306b236e216ae 100644 --- a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionFields.java +++ b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionFields.java @@ -213,11 +213,21 @@ protected static String getPartitionColumnName(String columnName, String transfo return columnName + "_bucket"; } + matcher = ICEBERG_BUCKET_PATTERN.matcher(transform); + if (matcher.matches()) { + return columnName + "_bucket"; + } + matcher = COLUMN_TRUNCATE_PATTERN.matcher(transform); if (matcher.matches()) { return columnName + "_trunc"; } + matcher = ICEBERG_TRUNCATE_PATTERN.matcher(transform); + if (matcher.matches()) { + return columnName + "_trunc"; + } + throw new UnsupportedOperationException("Unknown partition transform: " + transform); } diff --git a/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java b/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java index 95a4e543eb253..898006f1fcb72 100644 --- a/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java +++ b/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java @@ -297,7 +297,7 @@ public void testDeleteWithPartitionSpecEvolution() } @Test - public void testRenamePartitionColumn() + public void testRenameIdentityPartitionColumn() { assertQuerySucceeds("create table test_partitioned_table(a int, b varchar) with (partitioning = ARRAY['a'])"); assertQuerySucceeds("insert into test_partitioned_table values(1, '1001'), (2, '1002')"); @@ -314,6 +314,100 @@ public void testRenamePartitionColumn() assertQuerySucceeds("DROP TABLE test_partitioned_table"); } + @DataProvider(name = "transforms") + public String[][] transforms() + { + return new String[][] { + {"a int", "a"}, + {"a int", "bucket(a, 3)"}, + {"a int", "bucket(a, 3)', 'a"}, + {"a int", "truncate(a, 2)"}, + {"a int", "truncate(a, 2)', 'a', 'bucket(a, 3)"} + }; + } + + @DataProvider(name = "dateTimeTransforms") + public String[][] dateTimeTransforms() + { + return new String[][] { + {"a timestamp", "year(a)"}, + {"a timestamp", "month(a)"}, + {"a timestamp", "day(a)"}, + {"a timestamp", "hour(a)"}, + {"a timestamp", "a', 'month(a)"} + }; + } + + @Test(dataProvider = "transforms") + public void testRenamePartitionColumn(String[] transform) + { + assertQuerySucceeds("DROP TABLE IF EXISTS test_partitioned_table"); + assertQuerySucceeds(format("create table test_partitioned_table(%s) with (partitioning = ARRAY['%s'])", transform[0], transform[1])); + assertQuerySucceeds("insert into test_partitioned_table values(1), (2)"); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 1").getOnlyValue(), 1L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 2").getOnlyValue(), 1L); + + assertQuerySucceeds("alter table test_partitioned_table rename column a to d"); + assertQuerySucceeds("insert into test_partitioned_table values(1), (2), (3)"); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where d = 1").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where d = 2").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where d = 3").getOnlyValue(), 1L); + assertQueryFails("select a from test_partitioned_table", "line 1:8: Column 'a' cannot be resolved"); + + assertQuerySucceeds("alter table test_partitioned_table rename column d to e"); + assertQuerySucceeds("insert into test_partitioned_table values (3)"); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where e = 1").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where e = 2").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where e = 3").getOnlyValue(), 2L); + + assertQuerySucceeds("alter table test_partitioned_table rename column e to a"); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 1").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 2").getOnlyValue(), 2L); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 3").getOnlyValue(), 2L); + assertQuerySucceeds("insert into test_partitioned_table values (3)"); + assertEquals(getQueryRunner().execute("SELECT count(*) FROM test_partitioned_table where a = 3").getOnlyValue(), 3L); + assertQueryFails("select d from test_partitioned_table", "line 1:8: Column 'd' cannot be resolved"); + assertQueryFails("select e from test_partitioned_table", "line 1:8: Column 'e' cannot be resolved"); + assertQuerySucceeds("DROP TABLE test_partitioned_table"); + } + + @Test(dataProvider = "dateTimeTransforms") + public void testRenameDatetimePartitionColumn(String[] transform) + { + Session session = Session.builder(getSession()) + .setSystemProperty(LEGACY_TIMESTAMP, "false") + .build(); + assertQuerySucceeds("DROP TABLE IF EXISTS test_partitioned_table"); + assertQuerySucceeds(format("create table test_partitioned_table(%s) with (partitioning = ARRAY['%s'])", transform[0], transform[1])); + assertQuerySucceeds("insert into test_partitioned_table values(localtimestamp), (localtimestamp)"); + assertEquals(getQueryRunner().execute( + session, + "SELECT count(*) FROM test_partitioned_table where a <= localtimestamp").getOnlyValue(), 2L); + + assertQuerySucceeds("alter table test_partitioned_table rename column a to d"); + assertQuerySucceeds("insert into test_partitioned_table values(localtimestamp), (localtimestamp), (localtimestamp)"); + assertEquals(getQueryRunner().execute( + session, + "SELECT count(*) FROM test_partitioned_table where d <= localtimestamp").getOnlyValue(), 5L); + assertQueryFails("select a from test_partitioned_table", "line 1:8: Column 'a' cannot be resolved"); + + assertQuerySucceeds("alter table test_partitioned_table rename column d to e"); + assertQuerySucceeds("insert into test_partitioned_table values (localtimestamp)"); + assertEquals(getQueryRunner().execute( + session, + "SELECT count(*) FROM test_partitioned_table where e < localtimestamp").getOnlyValue(), 6L); + + assertQuerySucceeds("alter table test_partitioned_table rename column e to a"); + assertEquals(getQueryRunner().execute( + session, + "SELECT count(*) FROM test_partitioned_table where a < localtimestamp").getOnlyValue(), 6L); + assertQuerySucceeds("insert into test_partitioned_table values (localtimestamp)"); + assertEquals(getQueryRunner().execute(session, "SELECT count(*) FROM test_partitioned_table").getOnlyValue(), 7L); + assertQueryFails("select d from test_partitioned_table", "line 1:8: Column 'd' cannot be resolved"); + assertQueryFails("select e from test_partitioned_table", "line 1:8: Column 'e' cannot be resolved"); + assertQuerySucceeds("DROP TABLE test_partitioned_table"); + } + @Test public void testAddPartitionColumn() {