Repository navigation
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Reference Tables #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Reference Tables #6
Changes from all commits
72d61b72ee555876c0e9cc2f639cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Small fix on the numbers: no data file here has five rows, and the 2-row
a.parquetin the partitioned tables is 1087 B, with a 719 B footer. Something like "a Parquet file holding a few rows is about 1 KB, roughly two thirds of it footer" would match the files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
manifest_pathin the manifest lists and themetadata-logentries carry the prefix too. Also, onlylocationis spelledfile:///.manifest-list,manifest_path,file_pathandmetadata-logare allfile:/tmp/..., so the prefix as written above matches onlylocation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the code that wrote these tables be checked in next to them, even if CI never runs it? parquet-testing keeps its generators beside its fixtures (see the JSON and BSON section of
data/README.md).written-bygives the release, but not the writes, the table properties, or the steps after the write, like the pruning on line 141. Without that, it's hard to tell which bytes came straight from the writer and what steps were takenThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v<V>.metadata.jsonis the naming the spec gives for File System Tables. Metastore tables use<V>-<uuid>.metadata.json. Maybe scope this to how these tables were written.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which schema does the scan project at an earlier snapshot, the current one or the one named by the snapshot's
schema-id? Java's data-table scan and PyIceberg'sscan(snapshot_id=...)both use the snapshot's schema, and I couldn't find a spec clause that fixes it either way. Every table here has a single schema, so it doesn't matter today. But a table that adds a column between snapshots, like a v3 default-values table, would get different expected rows depending on the reading. Could the Procedure say "projected with the schema of the snapshot'sschema-id"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a check that follows each index entry through every retained snapshot, manifest list, manifest, and data file, verifying that live references exist, recorded file lengths match, and all snapshots remain readable? Intentionally pruned files in
metadata-logshould be excluded. Without automation, there is no ongoing guarantee that a future change does not silently break an older snapshot's artifact chain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea, I'd prefer to leave it out of scope of this PR as there's interest in introducing that in a follow up from @med9110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed two places where these tables follow the Java writer rather than the spec text. It would help to note them so an implementation of iceberg doesn't assert them. The v2 manifests, including the new
v2/partitioned-parquet-data, omit theschema-idkey-value entry, which the spec lists as required for v2. The v1 table carriesrefsand partitionfield-id, where the spec's v1 column is blank ("should be omitted").Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.