Core, Spark, Flink: Add table properties to metadata log entries - #16859
Core, Spark, Flink: Add table properties to metadata log entries#16859tomtongue wants to merge 9 commits into
Conversation
|
@szehon-ho @singhpk234, could I ask for your review if you have time? (@szehon-ho , tagging you because you have reviewed and authored several metadata-table changes. and @singhpk234, tagging you because this PR is closely modeled on The main thing I’d like feedback on is whether the user-facing table name/schema and the approach of reading retained metadata files are reasonable for a new read-only metadata table. If I should share this proposal and changes with the community dev thread, please let me know. |
c557032 to
81c20ec
Compare
|
@nastra (sorry for mentioning) I thought you might be a good reviewer because you have reviewed related metadata-table scan support and @szehon-ho @singhpk234, I'd also appreciate your review on this if you have time. |
4301cd9 to
b0c6875
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Still discussing in dev email thread and applying suggestions in the email thread. Please keep this pr opened. |
b0c6875 to
1f2585c
Compare
|
i think it sounds useful, is there a devlist discussion that you end up making?. any thoughts @singhpk234 ? |
|
@szehon-ho Thanks for taking a look at this PR and for your feedback. I’m currently working through the helpful comments from the dev-list discussion (many thanks to @tanmayrauth and @laskoviymishka as well). Apologies for my delayed response. I’ll address the feedback in the PR and reply to the dev-list thread by tomorrow. |
| case METADATA_LOG_ENTRIES: | ||
| return new MetadataLogEntriesTable(baseTable, metadataTableName); | ||
| case TABLE_PROPERTIES_LOG: | ||
| return new TablePropertiesLogTable(baseTable, metadataTableName); |
There was a problem hiding this comment.
i wonder, what if we includeproperties to the metadata_log table ?
There was a problem hiding this 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
propertiesis 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 thismetadata_log_entriestable in their operations), especially queries such asSELECT * ... 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
propertiesis not required, and to avoid unnecessary reads where possible. However, an unfilteredSELECT *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).
There was a problem hiding this comment.
Yes I think its cleaner. Yea my concern is mainly if there's something sensitive in table properties that we'd have to guard more now. Not sure if @singhpk234 has any thought on that
There was a problem hiding this comment.
I don't hold a very strong opinion here, I am fine either ways :), but sharing my thoughts here I feel as far as adding new columns to existing tables is concerned we have done that couple of times !
partitions
- spec_id — Adding specId for partitions metadata table #4516
- last_updated_at, last_updated_snapshot_id — Add last updated timestamp and snapshotId for partition table #7581
- total_data_file_size_in_bytes — Core: Add total data size to Partitions table #7920
- rename of last-updated timestamp column — Rename last updated timestamp column in PartitionsTable #8003
manifests / all_manifests
- content + added/existing/deleted_delete_files_count — Core: Add content and delete file counts to manifest tables #4764
- reference_snapshot_id (all_manifests) — Core: Add reference_snapshot_id column and filter to AllManifest table #4847
we can also just say its null ... if the file is missing, also our default max version is 100, i don't expect that much of an overhead !
Yea my concern is mainly if there's something sensitive in table properties that we'd have to guard more now
+1, infact in EMR we had to gaurd this all the way up to these metadata tables : https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-lf-limitations-cont.html , even paths are sensitive as sometime we have partition_col in the file name ... which can reveal which partition are there, but i feel this is more of the platform responsibility, from the glance of the table props defined nothing sensistive comes up to me, but folks can always add anything in the table prop, platforms may feel maintain an allowlist the props they wanna show though
There was a problem hiding this comment.
@singhpk234 Thanks for checking each concern and sharing the related PRs. @szehon-ho also, thanks for raising the masking concern.
From those PRs, adding properties as the optional column sounds reasonable and should have limited impact.
Regarding masking or allowlisting all versions of table props, I also think it's better handled at the platform or catalog layer as the table access management.
Based on those feedback, I'll proceed with adding an optional properties column to the existing metadata_log_entries table first. Also it will return null when a referenced metadata file is missing. If there's any issues while implementing it, I'll share with you!
There was a problem hiding this comment.
Thank you @singhpk234 for sharing the experience (especially wrt security) , im ok then to see the POC. I suppose by default the spark/flink engines wont mask anything
|
Sorry, do you have a link to the devlist? i seem not to find it. Also do we have any worry about masking any sensitive table property? I guess its not anything more than 'describe table extended' would show, but just some story around that? |
|
@szehon-ho this is the dev mailing list thread: https://lists.apache.org/thread/73tvsbmfnfhsow0nyq2lrblwbmbw00rb Regarding masking the table properties, I was also thinking about it, and as you mentioned, currently anyone can access the table property with |
1f2585c to
84982e4
Compare
|
In addition to those tests, I also tested querying > CREATE TABLE db.t (id bigint, data string) USING iceberg
> INSERT INTO db.t VALUES (1, 'a')
> SELECT * FROM db.t.metadata_log_entries
/*
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+---------------------------------------------------------+
|timestamp |file |latest_snapshot_id|latest_schema_id|latest_sequence_number|properties |
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+---------------------------------------------------------+
|2026-08-20 06:47:00.286|s3://warehouse/db/t/metadata/00000-4c91df99-f3eb-489c-950e-55415224b460.metadata.json|NULL |NULL |NULL |{owner -> spark, write.parquet.compression-codec -> zstd}|
|2026-08-20 06:47:40.675|s3://warehouse/db/t/metadata/00001-8298c092-34a4-4bd1-bf38-5e5c905f04b0.metadata.json|82363968092352335 |0 |1 |{owner -> spark, write.parquet.compression-codec -> zstd}|
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+---------------------------------------------------------+
*/
> ALTER TABLE db.t SET TBLPROPERTIES ('key1'='value')
> SELECT * FROM db.t.metadata_log_entries
/* the pair of key 1 and value is added in the latest snapshot
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+------------------------------------------------------------------------+
|timestamp |file |latest_snapshot_id|latest_schema_id|latest_sequence_number|properties |
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+------------------------------------------------------------------------+
|2026-08-20 06:47:00.286|s3://warehouse/db/t/metadata/00000-4c91df99-f3eb-489c-950e-55415224b460.metadata.json|NULL |NULL |NULL |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:47:40.675|s3://warehouse/db/t/metadata/00001-8298c092-34a4-4bd1-bf38-5e5c905f04b0.metadata.json|82363968092352335 |0 |1 |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:49:29.391|s3://warehouse/db/t/metadata/00002-41e0419e-beec-4ff1-944f-06c41e961f95.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key1 -> value, write.parquet.compression-codec -> zstd}|
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+------------------------------------------------------------------------+
*/
> ALTER TABLE db.t SET TBLPROPERTIES ('key2'='value2')
> SELECT * FROM db.t.metadata_log_entries
/* the pair of key 2 and value 2 is added in the latest snapshot
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
|timestamp |file |latest_snapshot_id|latest_schema_id|latest_sequence_number|properties |
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
|2026-08-20 06:47:00.286|s3://warehouse/db/t/metadata/00000-4c91df99-f3eb-489c-950e-55415224b460.metadata.json|NULL |NULL |NULL |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:47:40.675|s3://warehouse/db/t/metadata/00001-8298c092-34a4-4bd1-bf38-5e5c905f04b0.metadata.json|82363968092352335 |0 |1 |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:49:29.391|s3://warehouse/db/t/metadata/00002-41e0419e-beec-4ff1-944f-06c41e961f95.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key1 -> value, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:49:59.146|s3://warehouse/db/t/metadata/00003-ad8a3f69-a622-4c10-aabe-c083e724b15f.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key1 -> value, key2 -> value2, write.parquet.compression-codec -> zstd}|
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
*/
> ALTER TABLE db.t UNSET TBLPROPERTIES ('key1')
> SELECT * FROM db.t.metadata_log_entries
/* key 1 is removed in the latest snapshot
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
|timestamp |file |latest_snapshot_id|latest_schema_id|latest_sequence_number|properties |
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
|2026-08-20 06:47:00.286|s3://warehouse/db/t/metadata/00000-4c91df99-f3eb-489c-950e-55415224b460.metadata.json|NULL |NULL |NULL |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:47:40.675|s3://warehouse/db/t/metadata/00001-8298c092-34a4-4bd1-bf38-5e5c905f04b0.metadata.json|82363968092352335 |0 |1 |{owner -> spark, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:49:29.391|s3://warehouse/db/t/metadata/00002-41e0419e-beec-4ff1-944f-06c41e961f95.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key1 -> value, write.parquet.compression-codec -> zstd} |
|2026-08-20 06:49:59.146|s3://warehouse/db/t/metadata/00003-ad8a3f69-a622-4c10-aabe-c083e724b15f.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key1 -> value, key2 -> value2, write.parquet.compression-codec -> zstd}|
|2026-08-20 06:50:40.097|s3://warehouse/db/t/metadata/00004-7bd63877-5476-464b-afa6-2c2fdff2113b.metadata.json|82363968092352335 |0 |1 |{owner -> spark, key2 -> value2, write.parquet.compression-codec -> zstd} |
+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+
*/ |
|
@szehon-ho @singhpk234 I remove the |
tomtongue
changed the title
table_properties_log metadata table
tomtongue
changed the title
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice approach. Exposing property history through metadata_log_entries avoids adding a new table or changing the spec, and the core unit test covers the old-to-new case.
A few things from the review. Nothing blocking except the engine test:
- The Spark/Flink tests compare every row against the current property map, so they don’t really test history. They would still pass if the implementation ignored the older values. I’d change a property across metadata versions in one engine test and assert the different values. With that in, I’m comfortable approving.
- The title and description still refer to the earlier
table_properties_logdirection, while the PR now addspropertiestometadata_log_entries. Worth updating the description and example SQL to match. - I left a note in the existing thread about the per-entry metadata reads for
SELECT *and filtered queries. Not a blocker. - A few small inline comments on naming, error handling, and one indirect assertion.
Otherwise this looks good to me. Once the engine test is added and the description is updated, I’m good to approve.
|
|
||
| try { | ||
| return TableMetadataParser.read(io, metadataLogEntry.file()).properties(); | ||
| } catch (NotFoundException e) { |
There was a problem hiding this comment.
The null-on-missing-file behavior looks intentional and matches what you noted in the design thread — no issue there. What I'm less sure about is the other failure modes: TableMetadataParser.read can also throw RuntimeIOException (corrupt or partially-written metadata, permission denied), and those propagate and fail the whole scan. Since the use case is post-incident RCA, that's exactly when a historical file is most likely inconsistent. Do you want those to surface, or fold into the same null+warn path? Either's fine — just worth being deliberate about which.
There was a problem hiding this comment.
Thanks for calling this out. I chose the current behavior and I think it looks appropriate. Missing historical files can be expected due to metadata cleanup or else, so returning NULL with a warning is reasonable. A RuntimeIOException indicates corruption or an access issue, so it should surface instead of being treated as a missing file. I’ll keep the current behavior as is, if there's no other issues.
|
@laskoviymishka Thanks so much for the detailed review. I addressed your comments in the latest changes. |
…able-property-history assertion
faf3cf2 to
631c1c1
Compare
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups. This is close now.
The engine test I asked for is in for Spark and Flink, and it actually changes a property across metadata versions and checks the different values. That was the main gap for me. The PR description and example SQL also match the properties on metadata_log_entries direction now.
One small thing I’d still clean up: the existing testMetadataLogEntries still compares every row with the current property map. The dedicated history test covers the behavior now, so this is not blocking, but those assertions are a bit misleading as written. I’d either tighten them or leave a note that history is covered separately.
Also, Flink 2.3 seems to be missing the four base-case assertions that were added to the other Flink versions.
A couple of follow-ups, not blockers:
- missing historical metadata files are handled gracefully, but other IO errors still fail the query
- since this exposes full historical properties, it would be good to track the same surface in iceberg-go/PyIceberg and add a short docs note about sensitive or credential-like properties
I’m fine with this from my side. Before merge, I’d still like @szehon-ho or @singhpk234 to weigh in.
| null, | ||
| null), | ||
| null, | ||
| tableMetadata.properties()), |
There was a problem hiding this comment.
these new properties assertions all compare against tableMetadata.properties(), which is the current (final) map — and since testMetadataLogEntries never changes a property, every historical row happens to equal current. So this stays green even if loadTableProperties just returned current.properties() for every entry.
The real history coverage now lives in testMetadataLogEntriesPropertyHistory, so I wouldn't block on this. I'd either assert the concrete expected map per row here, or drop a one-line comment noting the properties are identical across entries in this test so it's clear the divergence check lives elsewhere. Same in the Flink testMetadataLogEntries copies. wdyt?
| } | ||
| } | ||
|
|
||
| @TestTemplate |
There was a problem hiding this comment.
v2.3 only picked up the new testMetadataLogEntriesPropertyHistory — the four getField("properties") assertions that landed in v1.20/2.1/2.2's testMetadataLogEntries didn't make it into this module. Same base commit, so it looks like a copy that got missed rather than a real version difference. I'd add the same four here so the base-case coverage matches across Flink versions.
|
|
||
| try { | ||
| return TableMetadataParser.read(io, metadataLogEntry.file()).properties(); | ||
| } catch (NotFoundException e) { |
There was a problem hiding this comment.
we only catch NotFoundException here, but TableMetadataParser.read rethrows other IO problems as RuntimeIOException — a present-but-unreadable file (permissions, a truncated/corrupt JSON, a transient read error) escapes and fails the whole query instead of degrading to null+warn like the missing-file case.
Since this is already best-effort, I'd broaden the catch (RuntimeException, or at least RuntimeIOException) and log the class+message. If we'd rather keep it narrow, worth a comment saying only missing files degrade gracefully. wdyt?
| */ | ||
| public class MetadataLogEntriesTable extends BaseMetadataTable { | ||
|
|
||
| private static final int PROPERTIES_FIELD_ID = 6; |
There was a problem hiding this comment.
PROPERTIES_FIELD_ID = 6 drives both the schema declaration and the findField(PROPERTIES_FIELD_ID) == null skip, but nothing checks the two agree — if a field ever gets inserted before properties, the constant silently points elsewhere and the skip mis-fires. A small static assertion that METADATA_LOG_ENTRIES_SCHEMA.findField(PROPERTIES_FIELD_ID) is non-null and named properties would keep them honest.
Tiny thing while we're here: LOG usually sits at the top of the constant block by convention.
4 participants
Overview
This PR adds an optional
propertiescolumn to the existingmetadata_log_entriesmetadata table. Each row exposes the table properties stored in that metadata version.The change is read-only. It does not change the Iceberg specification, table format, or write path. The visible history is limited to retained metadata files.
Motivation
iceberg-corealready stores the full property map in each metadata JSON file. However, users currently need to locate and parse historical files manually.This change makes property history queryable for auditing, debugging, and performance analysis.
Changes
This PR makes the following changes:
propertiesas an optionalmap<string, string>column.propertiesis not selected or filtered.NULLand logs a warning when a historical metadata file is missing.A property-only commit does not create a snapshot, so consecutive rows may have the same
latest_snapshot_id.Example
This shows the properties stored in each retained metadata version. See the full Spark output.
Selecting
properties, filtering on it, or usingSELECT *reads retained historical metadata files. When properties are not needed, project only the required columns:This avoids the additional reads needed to load historical properties.
Testing
AI Assistance
I used Claude Code and Codex to explore existing patterns and review the implementation, tests, and PR text. I reviewed the final changes and am responsible for them.
Appendix 1: Alternatives Considered
Record table properties into each snapshot summary at commit time.: This was the original design direction, and the full design is captured in Preserving Table Property History in Snapshot Summary. That approach would make property history available from
<table>.snapshotsand tie values directly to snapshot retention. However, it requires a write-path change and a spec addition for reserved snapshot-summary keys. It also needs pointer compression or another size-control mechanism to avoid repeating the full property map on every snapshot. The current PR avoids those concerns by exposing property history already present in retained metadata files.Add a new top-level property-history field to
TableMetadata.: Another option is to add a dedicated history structure to table metadata, such as a list of property-map versions or property deltas. That would make the history independent of metadata-log retention and could support direct point lookups without reading historical metadata files. However, it would introduce new persisted table metadata that every reader must preserve correctly, and it would require defining retention, compatibility, and upgrade behavior for that new structure. It is also heavier than necessary for this PR because Iceberg already stores the complete property map in each retainedmetadata.json. A read-only metadata table exposes that existing information without changing the table metadata format.