feat: Support metadata table "Metadata Log Entries"#846
Open
rshkv wants to merge 4 commits intoapache:mainfrom
Open
feat: Support metadata table "Metadata Log Entries"#846rshkv wants to merge 4 commits intoapache:mainfrom
rshkv wants to merge 4 commits intoapache:mainfrom
Conversation
rshkv
force-pushed
the
wr/metadata-log-entries
branch
from
bdb6372 to
067ada8
Compare
rshkv
commented
Dec 25, 2024
crates/iceberg/src/metadata_scan.rs
Outdated
Comment on lines
155
to
167
| fn snapshot_id_as_of_time( | ||
| table_metadata: &TableMetadataRef, | ||
| timestamp_ms_inclusive: i64, | ||
| ) -> Option<&SnapshotRef> { | ||
| let mut snapshot_id = None; | ||
| // The table metadata snapshot log is chronological | ||
| for log_entry in table_metadata.history() { | ||
| if log_entry.timestamp_ms <= timestamp_ms_inclusive { | ||
| snapshot_id = Some(log_entry.snapshot_id); | ||
| } | ||
| } | ||
| snapshot_id.and_then(|id| table_metadata.snapshot_by_id(id)) | ||
| } |
Contributor
Author
Contributor
Author
There was a problem hiding this comment.
This assumes that history() is chronologically ordered because that gets asserted TableMetadata#try_normalize.
rshkv
force-pushed
the
wr/metadata-log-entries
branch
from
067ada8 to
8d76183
Compare
rshkv
commented
Dec 25, 2024
crates/iceberg/src/metadata_scan.rs
Outdated
Comment on lines
211
to
323
| // Include the current metadata locaction and modification time in the table. This matches | ||
| // the Java implementation. Unlike the Java implementation, a current metadata location is | ||
| // optional here. In that case, we omit current metadata from the metadata log table. | ||
| if let Some(current_metadata_location) = &scan.metadata_location { | ||
| append_metadata_log_entry( | ||
| scan.metadata_ref.last_updated_ms(), | ||
| current_metadata_location, | ||
| ); | ||
| } |
Contributor
Author
rshkv
commented
Dec 25, 2024
Comment on lines
-1022
to
+1034
| .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) | ||
| .metadata_location(template_json_location) |
Contributor
Author
There was a problem hiding this comment.
Before this change, the location in the metadata log and the current location would both be ../metadata/v1.json.
I wanted to have a distinction so we can assert that metadata_log_entries includes the current metadata location, even if not in the metadata log.
Member
|
#822 has been merged, let's move on! |
rshkv
force-pushed
the
wr/metadata-log-entries
branch
from
8d76183 to
395ff1f
Compare
Contributor
Author
|
Thank you, @Xuanwo. This is rebased and ready for review. |
rshkv
force-pushed
the
wr/metadata-log-entries
branch
from
76c2bf2 to
439c529
Compare
rshkv
commented
Jan 2, 2025
crates/iceberg/src/metadata_table.rs
Outdated
Comment on lines
454
to
543
| -- validity: | ||
| -- validity: |
Contributor
Author
There was a problem hiding this comment.
This was failing for me locally.
rshkv
commented
Jan 2, 2025
| pub mod spec; | ||
|
|
||
| pub mod metadata_scan; | ||
| pub mod metadata_table; |
Contributor
Author
There was a problem hiding this comment.
Follow-up to this comment: #822 (comment)
Don't need to do in this PR. Up to you.
rshkv
commented
Jan 2, 2025
| /// Creates a metadata table which provides table-like APIs for inspecting metadata. | ||
| /// See [`MetadataTable`] for more details. | ||
| pub fn metadata_table(self) -> MetadataTable { | ||
| pub fn metadata_table(&self) -> MetadataTable<'_> { |
Contributor
Author
There was a problem hiding this comment.
Follow-up to this comment: #822 (comment)
Also not necessary to do in this PR.
Member
|
Let's get #863 merge first. |
Contributor
+1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Re #823. This adds support for the Metadata Log Entries metadata table.
This is building on @xxchan's unmerged #822. I'll update and rebase this PR when #822 merges.✅Metadata Log Entries is the metadata log with the latest snapshot per metadata file.
Reference implementations: