Core: Allow write.metadata.previous-versions-max to be 0#16796
Core: Allow write.metadata.previous-versions-max to be 0#16796smaheshwar-pltr wants to merge 4 commits into
Conversation
| | write.summary.partition-limit | 0 | Includes partition-level summary stats in snapshot summaries if the changed partition count is less than this limit | | ||
| | write.metadata.delete-after-commit.enabled | false | Controls whether to delete the oldest **tracked** version metadata files after each table commit. See the [Remove old metadata files](maintenance.md#remove-old-metadata-files) section for additional details | | ||
| | write.metadata.previous-versions-max | 100 | The max number of previous version metadata files to track | | ||
| | write.metadata.previous-versions-max | 100 | The max number of previous version metadata files to track. A value of `0` keeps no previous metadata files (an empty `metadata-log`) | |
There was a problem hiding this comment.
Pointing out that I'm not including a spec change in this PR because I kind of think
A list (optional) of timestamp and metadata file location pairs that encodes changes to the previous metadata files for the table. Each time a new metadata file is created, a new entry of the previous metadata file location should be added to the list. Tables can be configured to remove oldest metadata log entries and keep a fixed-size log of the most recent entries after a commit.
from https://iceberg.apache.org/spec/#table-metadata is worded consistently with a value of 0?
…ded-file delete Addresses review on PR apache#16796: drop the trackedMetadataFiles helper and restore the original entry-level removeAll (MetadataLogEntry equality) over Sets.newHashSet(base.previousFiles()), with the original comment. The max=0 behaviour is now an explicitly-guarded additive step: delete base's superseded metadata file only when the new metadata no longer references it (not its current location and not in its log). This keeps the method a no-op for previous-versions-max >= 1 and for base == metadata (a public-API contract the helper version regressed), and is null-safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins the public-API contract that the superseded-file deletion must never remove the live current metadata file: with base and metadata sharing a location and an empty log (the previous-versions-max=0 shape), nothing is deleted. This guards the !superseded.equals(metadata.metadataFileLocation()) condition, which no existing test covered. The null-base-location path is already exercised by noFailureWhenBulkDeletingMetadataFiles, so no separate test is added for it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
85e3b32 to
98ea1dd
Compare
| int maxSize = | ||
| Math.max( | ||
| 1, | ||
| 0, |
There was a problem hiding this comment.
Pointing out that a negative value for this property now corresponds to an empty log - previously, it was a log of maximum size one. This is technically a break, but that felt fine to me?
Closes #16797, if folks are aligned with that feature request in the first place (please let me know your thoughts there!)