-
Notifications
You must be signed in to change notification settings - Fork 850
feat(dataset)!: expose manifest_size on Version #9102
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,6 +278,14 @@ pub struct Version { | |
|
|
||
| /// Key-value pairs of metadata. | ||
| pub metadata: BTreeMap<String, String>, | ||
|
|
||
| /// Size of the manifest file for this version, in bytes, if known. | ||
| /// | ||
| /// On wide tables (many columns and/or fragments) the manifest is rewritten | ||
| /// in full on every commit; exposing its size per version makes metadata | ||
| /// growth observable via `list_versions`. | ||
| #[serde(default)] | ||
| pub manifest_size: Option<u64>, | ||
| } | ||
|
|
||
| /// A lightweight reference to an attached dataset version, which could be used to uniquely identify a version. | ||
|
|
@@ -295,6 +303,7 @@ impl From<&Manifest> for Version { | |
| version: m.version, | ||
| timestamp: m.timestamp(), | ||
| metadata: m.summary().into(), | ||
| manifest_size: None, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2611,8 +2620,13 @@ impl Dataset { | |
| .commit_handler | ||
| .list_manifest_locations(&self.base, &self.object_store, false) | ||
| .try_filter_map(|location| async move { | ||
| match read_manifest(&self.object_store, &location.path, location.size).await { | ||
| Ok(manifest) => Ok(Some(Version::from(&manifest))), | ||
| let manifest_size = location.size; | ||
| match read_manifest(&self.object_store, &location.path, manifest_size).await { | ||
| Ok(manifest) => { | ||
| let mut version = Version::from(&manifest); | ||
| version.manifest_size = manifest_size; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assignment and the Python key have no corresponding committed regression coverage; the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 10d93dae4: |
||
| Ok(Some(version)) | ||
| } | ||
| Err(e) => Err(e), | ||
| } | ||
| }) | ||
|
|
||
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.
The runtime dictionary now gains
manifest_size, but the publicVersionTypedDictinpython/python/lance/dataset.pystill declares onlyversion,timestamp, andmetadata. Typed callers therefore cannot use the advertised key: Pyright reports"manifest_size" is not a defined key in "Version". Please addmanifest_size: int | Noneand document the optional/unknown case so the public Python type contract matches this binding.Reproducer
Run against this head with the following file:
uv run pyright gate_manifest_size_repro.pyreports: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.
Fixed in 10d93dae4:
Versionnow declaresmanifest_size: int | None, and the original Pyright reproducer completes with zero errors.