-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add Iceberg support for ALTER TABLE ... SET PROPERTIES #12161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,6 +238,25 @@ otherwise the procedure will fail with similar message: | |
| ``Retention specified (1.00d) is shorter than the minimum retention configured in the system (7.00d)``. | ||
| The default value for this property is ``7d``. | ||
|
|
||
| .. _iceberg-alter-table-set-properties: | ||
|
|
||
| ALTER TABLE SET PROPERTIES | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| The connector supports modifying the properties on existing tables using | ||
| :ref:`ALTER TABLE SET PROPERTIES <alter-table-set-properties>`. | ||
|
|
||
| The following table properties can be updated after a table is created: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we / should we have documented somewhere this statement used to retrieve the "updatable" table properties?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a separate doc page for system tables or information_schema? I couldn't find one, but I feel like that's where it should go |
||
|
|
||
| * ``format`` | ||
| * ``format_version`` | ||
|
|
||
| For example, to update a table from v1 of the Iceberg specification to v2: | ||
|
|
||
| .. code-block:: sql | ||
|
|
||
| ALTER TABLE table_name SET PROPERTIES format_version = 2; | ||
|
|
||
| .. _iceberg-type-mapping: | ||
|
|
||
| Type mapping | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,8 @@ | |
| import static io.trino.testing.sql.TestTable.randomTableSuffix; | ||
| import static io.trino.tpch.TpchTable.NATION; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| import static org.testng.Assert.assertEquals; | ||
|
|
||
| public class TestIcebergV2 | ||
| extends AbstractTestQueryFramework | ||
|
|
@@ -172,6 +174,39 @@ public void testV2TableWithEqualityDelete() | |
| assertQuery("SELECT nationkey FROM " + tableName, "SELECT nationkey FROM nation WHERE regionkey != 1"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUpgradeTableToV2FromTrino() | ||
| { | ||
| String tableName = "test_upgrade_table_to_v2_from_trino_" + randomTableSuffix(); | ||
| assertUpdate("CREATE TABLE " + tableName + " WITH (format_version = 1) AS SELECT * FROM tpch.tiny.nation", 25); | ||
| assertEquals(loadTable(tableName).operations().current().formatVersion(), 1); | ||
| assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES format_version = 2"); | ||
| assertEquals(loadTable(tableName).operations().current().formatVersion(), 2); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please verify via that the change of the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added And no, I don't think it does
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just thinking that it would be good to have it in case of eventual regressions. |
||
| assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation"); | ||
| } | ||
|
|
||
| @Test | ||
|
alexjo2144 marked this conversation as resolved.
Outdated
|
||
| public void testDowngradingV2TableToV1Fails() | ||
| { | ||
| String tableName = "test_downgrading_v2_table_to_v1_fails_" + randomTableSuffix(); | ||
| assertUpdate("CREATE TABLE " + tableName + " WITH (format_version = 2) AS SELECT * FROM tpch.tiny.nation", 25); | ||
| assertEquals(loadTable(tableName).operations().current().formatVersion(), 2); | ||
| assertThatThrownBy(() -> query("ALTER TABLE " + tableName + " SET PROPERTIES format_version = 1")) | ||
| .hasMessage("Failed to commit new table properties") | ||
| .getRootCause() | ||
| .hasMessage("Cannot downgrade v2 table to v1"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUpgradingToInvalidVersionFails() | ||
| { | ||
| String tableName = "test_upgrading_to_invalid_version_fails_" + randomTableSuffix(); | ||
| assertUpdate("CREATE TABLE " + tableName + " WITH (format_version = 2) AS SELECT * FROM tpch.tiny.nation", 25); | ||
| assertEquals(loadTable(tableName).operations().current().formatVersion(), 2); | ||
| assertThatThrownBy(() -> query("ALTER TABLE " + tableName + " SET PROPERTIES format_version = 42")) | ||
| .hasMessage("Unable to set catalog 'iceberg' table property 'format_version' to [42]: format_version must be between 1 and 2"); | ||
| } | ||
|
|
||
| private void writeEqualityDeleteToNationTable(Table icebergTable) | ||
| throws Exception | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mosabua mind taking a look at the doc update?