tomtongue commented on code in PR #16859:
URL: https://github.com/apache/iceberg/pull/16859#discussion_r3809119990
##########
core/src/main/java/org/apache/iceberg/MetadataTableUtils.java:
##########
@@ -70,6 +70,8 @@ private static Table createMetadataTableInstance(
return new SnapshotsTable(baseTable, metadataTableName);
case METADATA_LOG_ENTRIES:
return new MetadataLogEntriesTable(baseTable, metadataTableName);
+ case TABLE_PROPERTIES_LOG:
+ return new TablePropertiesLogTable(baseTable, metadataTableName);
Review Comment:
@singhpk234 Thanks so much for the review and suggestion. I think adding
`properties` to `metadata_log_entries` is feasible, and it has the benefit of
not introducing another metadata table.
Regarding adding the `properties` column to the table, I believe there are a
few concerns:
* Even if `properties` is added as an optional field, it changes the schema
of an existing metadata table. This may affect existing users (I don't think
many use this `metadata_log_entries` table in their operations), especially
queries such as `SELECT * ... UNION ...` that rely on the current number or
order of columns.
* To return historical properties, we still need to read the metadata file
referenced by each entry. We need to decide how to handle missing or unreadable
files, for example whether to fail the whole scan or keep the row with
`properties = null`. This is the same read-cost and failure-handling concern
raised on the dev list.
* This also introduces additional I/O to obtain table properties by
accessing each metadata file. I plan to make the loading projection-aware so
that historical metadata files are not read when `properties` is not required,
and to avoid unnecessary reads where possible. However, an unfiltered `SELECT
*` will still need to read all retained metadata files.
I would keep the current semantics of the existing columns and only add the
new `properties` field.
I’ll try implementing this approach to compare between two patterns of
implementations. Does this direction sound reasonable? Please let me know if
you see any other concerns (in addition to the concern @szehon-ho mentioned
below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
