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 extra_properties to iceberg table properties #20393

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Add extra_properties to iceberg table properties #20393

merged 3 commits into from
Oct 31, 2024
+322 −70

Conversation

ndrluis
Copy link
Member

@ndrluis ndrluis commented Jan 16, 2024 •
edited
Loading

Description

This PR allows us to pass through additional properties to Iceberg when creating a table in Trino.
The additional properties can be provided in the following format

extra_properties = MAP(ARRAY['extra.property.one', 'extra.property.two'], ARRAY['one', 'two'])

The extra properties will not be exposed in SHOW CREATE.

The purpose of this feature is to add properties that can be used by catalogs or other tools. For example, in Tabular, there is a property to optimize tables. For our use case, we want to create our tables with the optimizer.enabled value set to False, and we cannot achieve this without a way to handle arbitrary properties.

Additional context and related issues

Fixes #17427

Release notes

(x) Release notes are required, with the following suggested text:

# Iceberg
* Add support for `extra_properties` table properties. ({issue}`17427`)

@cla-bot cla-bot bot added the cla-signed label Jan 16, 2024
@github-actions github-actions bot added docs iceberg Iceberg connector labels Jan 16, 2024
mosabua
mosabua previously requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Docs changes are fine. Java code seems to be using wrong style. Please update - see https://trino.io/development/#code-style

oneonestar
oneonestar suggested changes Jan 17, 2024
View reviewed changes
Copy link
Member

@oneonestar oneonestar left a comment

Choose a reason for hiding this comment

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

Looks like this PR will also allow ALTER TABLE SET PROPERTIES extra_properties. Please consider adding this to the documentation.

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

regarding maven-checks failure 🔴

do the following before submitting the PR to ensure that the code you are changing follows the code conventions:

./mvnw clean install -P errorprone-compiler -DskipTests -nsu -pl :trino-iceberg

@github-actions github-actions bot added tests:hive hive Hive connector labels Jan 17, 2024
@ndrluis
Copy link
Member Author

ndrluis commented Jan 17, 2024

@findinpath @oneonestar @mosabua I made the requested changes!

@ndrluis ndrluis force-pushed the iceberg-extra-properties branch 2 times, most recently from e9b9e2c to bcc1ed3 Compare January 17, 2024 19:20
@ndrluis ndrluis removed hive Hive connector tests:hive labels Jan 17, 2024
@ndrluis ndrluis force-pushed the iceberg-extra-properties branch 2 times, most recently from ce2d3bf to 1e6de9c Compare January 17, 2024 21:26
@github-actions github-actions bot added the stale label Sep 18, 2024
@meysampg
Copy link

meysampg commented Oct 1, 2024

Hi. Is there any plan to merge this PR?

@github-actions github-actions bot removed the stale label Oct 1, 2024
@mosabua
Copy link
Member

mosabua commented Oct 8, 2024

@ebyhr @findinpath is this ready for merge?

@peterklingelhofer
Copy link

peterklingelhofer commented Oct 27, 2024 •
edited
Loading

Hopeful this can be merged somewhat soon. Being able to access these properties is essential if you're trying to do parallel UPSERT (MERGE INTO in Iceberg), i.e aws/aws-sdk-pandas#2651 (comment)

@sopel39
Copy link
Member

sopel39 commented Oct 29, 2024

I've rebased and addressed some of the comments. @ebyhr could you PTAL?

@ebyhr
Copy link
Member

ebyhr commented Oct 29, 2024

Sure, let me review tomorrow.

@sopel39 sopel39 force-pushed the iceberg-extra-properties branch 2 times, most recently from 9cd7df1 to 981c6a7 Compare October 29, 2024 14:18
@sopel39
Copy link
Member

sopel39 commented Oct 29, 2024

@ebyhr I've added iceberg.allowed-extra-properties to allow setting only specified extra properties.

ebyhr
ebyhr reviewed Oct 29, 2024
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java Show resolved Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
sopel39
sopel39 reviewed Oct 30, 2024
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

ac

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Show resolved Hide resolved
@sopel39 sopel39 requested a review from ebyhr October 30, 2024 11:49
@ndrluis @Priyansh121096 @sopel39
Add extra_properties to iceberg table properties
8719414 
Some policies require to add extra properties to Iceberg tables
(e.g. for tracking table origin).

Co-authored-by: Priyansh121096 <[email protected]>
@sopel39
Copy link
Member

sopel39 commented Oct 30, 2024

@ebyhr ptal

@sopel39 sopel39 merged commit 9403964 into trinodb:master Oct 31, 2024
9 of 26 checks passed
@github-actions github-actions bot added this to the 465 milestone Oct 31, 2024
@ndrluis ndrluis deleted the iceberg-extra-properties branch October 31, 2024 14:04
@sopel39 sopel39 mentioned this pull request Oct 31, 2024
Merged
@mosabua mosabua mentioned this pull request Nov 1, 2024
Open
@ebyhr ebyhr mentioned this pull request Nov 5, 2024
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

@ebyhr ebyhr ebyhr approved these changes

@sopel39 sopel39 sopel39 approved these changes

@zhmaeff zhmaeff zhmaeff approved these changes

@oneonestar oneonestar oneonestar approved these changes

@mosabua mosabua mosabua left review comments

@findepi findepi Awaiting requested review from findepi

Assignees

@ndrluis ndrluis

Labels
cla-signed docs iceberg Iceberg connector
Milestone
465
Development

Successfully merging this pull request may close these issues.

Add extra_properties to Iceberg table properties