Support setting Iceberg table format version#11880
Conversation
There was a problem hiding this comment.
From a new user perspective I think it would be useful to have a very high-level description about what 1 and 2 actually mean.
There was a problem hiding this comment.
to use for tables created by Trino
There was a problem hiding this comment.
for tables created by Trino
There was a problem hiding this comment.
this applies to CREATE TABLE, but also to SHOW CREATE, so should just describe what it is:
"Iceberg table format version"
|
What's the benefit? Until v2 is fully supported, i don't think we should merge this. |
There are some spec changes in v2 besides merge-on-read row level deletes. https://iceberg.apache.org/spec/#version-2 A user might want v2 tables for those reasons, but we can combine this with #11886 if you'd like |
There was a problem hiding this comment.
This is compatible, but please follow up to create v2 by default.
We are implementing v2 deletes, and are currently not working on copy-on-write delete mode (as required for v1), so v2 are better for end users.
There was a problem hiding this comment.
If copy-on-write isn't implemented, do consider defaulting write.format.default to merge-on-read then. Particularly for users who might interact with the table via other engines.
There was a problem hiding this comment.
Actually there are 3 configurations coming up with respect to copy-on-write vs merge-on-read. These technically only apply to v2, but if you're implementing merge-on-read only, then I'd suggest considering updating these properties from the upstream default of copy-on-write:
write.delete.modewrite.update.modewrite.merge.mode
There was a problem hiding this comment.
Yep, at the moment neither merge-on-read or copy-on-write are supported so I didn't add those three options in here, but they are in the PR adding support for merge-on-read deletes: 8ebba99#diff-2af3e19a6b656640a7d0bb73114ef224953a2efa04e569b1fe4da953b2cc6d15R411
There was a problem hiding this comment.
this applies to CREATE TABLE, but also to SHOW CREATE, so should just describe what it is:
"Iceberg table format version"
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java
Outdated
Show resolved
Hide resolved
alexjo2144
force-pushed
the
iceberg/format-version
branch
from
65a6a5c to
e529efa
Compare
|
@findepi addressed comments, @findinpath added to the docs a little, explaining that v2 is required for row level deletes and linking the Iceberg documentation |
There was a problem hiding this comment.
https://iceberg.apache.org/spec/#version-2 would be a correct link.
However, i am fine with omitting the whole sentence, we can consider this out of scope of this document.
From Trino user perspective, we should say that "2" is required for DML.
There was a problem hiding this comment.
From Trino user perspective, we should say that "2" is required for DML
I was going to put that, but not until after DML is in
There was a problem hiding this comment.
Removed the last sentence
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java
Outdated
Show resolved
Hide resolved
alexjo2144
force-pushed
the
iceberg/format-version
branch
from
e529efa to
dba240d
Compare
findepi
added
the
no-release-notes
4 participants
Description
Allows setting the table format for newly created Iceberg tables. Defaults to v1.
Feature
Iceberg connector
Allows creating either v1 or v2 Iceberg tables
Related issues, pull requests, and links
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: