Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Iceberg support for ALTER TABLE ... SET PROPERTIES #12161

Merged
merged 1 commit into from
May 5, 2022
Merged

Add Iceberg support for ALTER TABLE ... SET PROPERTIES #12161

merged 1 commit into from
May 5, 2022

Conversation

alexjo2144
Copy link
Member

@alexjo2144 alexjo2144 commented Apr 27, 2022 •
edited
Loading

Description

The two properties which can be set are format and format_version.

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Iceberg connector

How would you describe this change to a non-technical end user or system administrator?

Allows updating specific metadata for an Iceberg table. The format_version, and the format of the underlying data files.

Related issues, pull requests, and links

Fixes: #12138

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Add support for updating the `format` and `format_version` table properties using `ALTER TABLE ... SET PROPERTIES`. This allows for updating tables from v1 of the Iceberg specification to v2.

@cla-bot cla-bot bot added the cla-signed label Apr 27, 2022
findepi
findepi reviewed Apr 28, 2022
View reviewed changes
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated
case FORMAT_VERSION_PROPERTY:
updateProperty(updateProperties, FORMAT_VERSION, propertyValue, formatVersion -> Integer.toString((int) formatVersion));
break;
case PARTITIONING_PROPERTY:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should allow changing partitioning, let's have a TODO+issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12174

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Show resolved Hide resolved
@findinpath
Copy link
Contributor

Add a product test to the class io.trino.tests.product.iceberg.TestIcebergSparkCompatibility for checking the compatibility with Spark on updating table properties.

@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch from ccb954b to 5c98951 Compare April 28, 2022 14:23
@alexjo2144
Copy link
Member Author

Add a product test to the class io.trino.tests.product.iceberg.TestIcebergSparkCompatibility for checking the compatibility with Spark on updating table properties.

I was thinking about that, but wasn't sure what exactly was testable from the Spark side. The test I added to TestIcebergV2 is engine-agnostic, using the Iceberg lib directly to check the format version. I don't think there's a SparkSQL query to ask for the format version though.

@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch 2 times, most recently from f16fd6e to 30059ee Compare April 28, 2022 14:54
@alexjo2144
Copy link
Member Author

AC Thanks @findepi @findinpath

Also added documentation

alexjo2144
alexjo2144 commented Apr 28, 2022
View reviewed changes
docs/src/main/sphinx/connector/iceberg.rst
@@ -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:
Copy link
Member Author

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?

@github-actions github-actions bot added the docs label Apr 28, 2022
@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch from 30059ee to d1d1eca Compare April 28, 2022 16:56
@findepi findepi requested a review from findinpath April 29, 2022 10:00
@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch from d1d1eca to 246adbb Compare April 29, 2022 15:56
@alexjo2144
Copy link
Member Author

Thanks @findinpath, added those test cases

@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch from 246adbb to 56bb7f5 Compare May 3, 2022 14:55
findinpath
findinpath reviewed May 4, 2022
View reviewed changes
docs/src/main/sphinx/connector/iceberg.rst
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

SELECT * FROM system.metadata.table_properties where catalog_name = 'iceberg';

Do we / should we have documented somewhere this statement used to retrieve the "updatable" table properties?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

findinpath
findinpath approved these changes May 4, 2022
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please verify via

assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation");

that the change of the format_version has no negative outcome for the end user?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

And no, I don't think it does

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@alexjo2144
Add Iceberg support for ALTER TABLE ... SET PROPERTIES
4bc37bb 
The two properties which can be set are `format` and `format_version`.
@alexjo2144 alexjo2144 force-pushed the iceberg/set-properties branch from 56bb7f5 to 4bc37bb Compare May 4, 2022 17:59
@findinpath findinpath self-requested a review May 4, 2022 19:35
@findepi findepi merged commit 9fca255 into trinodb:master May 5, 2022
@github-actions github-actions bot added this to the 380 milestone May 5, 2022
@findepi findepi mentioned this pull request May 5, 2022
Closed
@alexjo2144 alexjo2144 deleted the iceberg/set-properties branch May 5, 2022 13:09
@mosabua mosabua mentioned this pull request May 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findinpath findinpath findinpath approved these changes

@findepi findepi findepi approved these changes

@homar homar homar approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed docs
Milestone
380
Development

Successfully merging this pull request may close these issues.

Support migrating Iceberg v1 tables to v2
4 participants
@alexjo2144 @findinpath @homar @findepi