Uh oh!
There was an error while loading. Please reload this page.
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
81c20ecComparetomtongue
commented
Jun 24, 2026
@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
b0c6875CompareThis 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
1f2585cCompareszehon-ho
commented
Aug 17, 2026
i think it sounds useful, is there a devlist discussion that you end up making?. any thoughts @singhpk234 ? |
tomtongue
commented
Aug 18, 2026
@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? |
tomtongue
commented
Aug 19, 2026
@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
84982e4CompareIn addition to those tests, I also tested querying > CREATE TABLE db.t (id bigint, data string) USING iceberg
>INSERT INTOdb.tVALUES (1, 'a')
>SELECT*FROMdb.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.tSET TBLPROPERTIES ('key1'='value')
>SELECT*FROMdb.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.tSET TBLPROPERTIES ('key2'='value2')
>SELECT*FROMdb.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*FROMdb.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} |+-----------------------+-------------------------------------------------------------------------------------+------------------+----------------+----------------------+----------------------------------------------------------------------------------------+*/ |
Uh oh!
There was an error while loading. Please reload this page.
@szehon-ho@singhpk234 I remove the |
table_properties_log metadata table
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.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tomtongue
commented
Aug 25, 2026
@laskoviymishka Thanks so much for the detailed review. I addressed your comments in the latest changes. |
…able-property-history assertion
faf3cf2 to
631c1c1Compare
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.