Repository navigation
Conversation
github-actions
Bot
added
A-deps
📝 WalkthroughWalkthroughAdds raw manifest protobuf reading and separates semantic conversion. Extends ChangesManifest inspection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LanceToolsArgs
participant LanceToolManifest
participant ObjectStore
participant Writer
User->>LanceToolsArgs: run table manifest --source
LanceToolsArgs->>LanceToolManifest: show_table_manifest
LanceToolManifest->>ObjectStore: read manifest and sections
ObjectStore-->>LanceToolManifest: return manifest data
LanceToolManifest->>Writer: write formatted report
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b50c8d0 to
a20868d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-tools/src/manifest.rs (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten
LanceToolManifestvisibility topub(crate).
LanceToolManifestis only used internally (via thepub(crate)show_table_manifestand tests); its fields are private and it is not part of an intended public API. Sincelib.rsre-exports this module (pub mod manifest;), leaving itpubleaks an internal type onto the crate's public surface.As per coding guidelines: "Prefer
pub(crate)overpubfor crate-internal items, and usepub usere-exports for the actual public API surface."♻️ Proposed change
-pub struct LanceToolManifest { +pub(crate) struct LanceToolManifest {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 34 - 41, Change the visibility of the LanceToolManifest struct from pub to pub(crate), keeping its fields and implementation unchanged so the internally used type is no longer exposed as part of the public API.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/lance-tools/src/manifest.rs`:
- Around line 34-41: Change the visibility of the LanceToolManifest struct from
pub to pub(crate), keeping its fields and implementation unchanged so the
internally used type is no longer exposed as part of the public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: d8f30995-630a-4605-9599-87d5700ab8d4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust/lance-table/src/io/manifest.rsrust/lance-tools/Cargo.tomlrust/lance-tools/README.mdrust/lance-tools/src/cli.rsrust/lance-tools/src/lib.rsrust/lance-tools/src/manifest.rs
a20868d to
fecb66a
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
rust/lance-tools/src/manifest.rs-608-618 (1)
608-618: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
uuid_bytesprints a length, not bytes.In the parse-failure branch — the one path an operator uses to diagnose a corrupt index entry — this emits the byte count (e.g.
16), which conveys nothing about the malformed uuid. Rename the key to reflect a count, and consider hex-dumping the raw bytes instead.♻️ Proposed change
write_kv( f, 8, - "uuid_bytes", + "uuid_byte_len", &index .raw .uuid .as_ref() .map(|uuid| uuid.uuid.len().to_string()) .unwrap_or_else(|| "absent".to_string()), )?;As per coding guidelines: "for numeric parameters state whether the value is an id, count, index, etc." and "Use precise domain terminology."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 608 - 618, Update the write_kv call in the parse-failure manifest formatting path to rename the uuid_bytes key to a precise byte-count name such as uuid_byte_count, matching the existing length value; if retaining diagnostic detail, add a separate hex representation of the raw UUID bytes rather than labeling the count as bytes.Source: Coding guidelines
rust/lance-tools/src/manifest.rs-855-855 (1)
855-855: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRender
update_modeas the enum variant name.
update.update_modeis a prost open enum field (i32), so.to_string()writes0/1, which obscures the update mode in manifest dumps. Use the generated accessor/enumerator and only fall back to the numeric value for unknown discriminators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` at line 855, Update the manifest rendering in the write_kv call for update.update_mode to resolve the prost enum discriminator through its generated accessor or enumerator, emitting the enum variant name for known values and retaining the numeric discriminator only for unknown values.Source: Coding guidelines
🧹 Nitpick comments (5)
rust/lance-tools/src/manifest.rs (5)
111-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the recognized manifest layouts and the
Nonecase.The inference rules (
<base>/_latest.manifestand<base>/_versions/*.manifest) and the meaning ofNone(base not inferable → external transaction files cannot be resolved, per Lines 173-178) are non-obvious to a reader. Also worth noting that a root-level manifest yieldsNonebecausepath_from_partsrejects an empty prefix.📝 Proposed doc comments
+/// Infers the dataset base directory from a manifest path. +/// +/// Recognizes two layouts: `<base>/_latest.manifest` and `<base>/_versions/<n>.manifest`. +/// Returns `None` when the path matches neither layout, or when the inferred base would be +/// empty (a manifest sitting at the store root). Callers must then report external +/// transaction files as unresolvable rather than guessing a base. fn infer_dataset_base_from_manifest_path(path: &Path) -> Option<Path> {As per coding guidelines: "Add doc comments to magic constants, thresholds, and non-obvious transformation functions" and "Comment fallback or guard code paths with when they trigger and why they exist."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 111 - 133, Add doc comments for infer_dataset_base_from_manifest_path and path_from_parts documenting the recognized <base>/_latest.manifest and <base>/_versions/*.manifest layouts, that None means the dataset base cannot be inferred and external transaction files cannot be resolved, and that root-level manifests return None because path_from_parts rejects an empty prefix.Source: Coding guidelines
73-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the entire manifest protobuf for two presence checks.
raw_manifestis retained solely to testtimestamp.is_some()(Line 239) anddata_format.is_some()(Line 249). The clone at Line 75 duplicates the whole proto — schema fields plus every fragment — doubling peak memory on large tables. Capture the two booleans instead and drop the field.♻️ Proposed refactor
- let raw_manifest = - read_manifest_proto(&source.object_store, &source.manifest_path, None).await?; - let manifest = Manifest::try_from(raw_manifest.clone())?; + let raw_manifest = + read_manifest_proto(&source.object_store, &source.manifest_path, None).await?; + let has_timestamp = raw_manifest.timestamp.is_some(); + let has_data_format = raw_manifest.data_format.is_some(); + let manifest = Manifest::try_from(raw_manifest)?;Then replace the
raw_manifest: pb::Manifestfield withhas_timestamp: bool/has_data_format: booland update Lines 239 and 249.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 73 - 97, In the manifest construction flow, remove the full protobuf clone and replace retained raw_manifest presence checks with boolean flags captured from the original raw manifest before conversion. Add has_timestamp and has_data_format fields to the containing struct, initialize them in this constructor, and update the timestamp/data-format checks in the relevant methods to use those flags.
1269-1285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
Noneinference case and the error-rendering branches.Both existing unit tests cover only successful inference. Uncovered: paths that match neither layout (a bare
foo.manifest, a*.manifestnot under_versions, a root-level_latest.manifest), and theindices_error:/transaction_error:output branches at Lines 548 and 683, which no test reaches.💚 Proposed additional test
#[test] fn test_infer_dataset_base_returns_none_for_unrecognized_paths() { for path in [ "foo.manifest", "dataset/other/1.manifest", "dataset/_versions/1.txt", "_latest.manifest", ] { assert!( infer_dataset_base_from_manifest_path(&Path::from(path)).is_none(), "expected no dataset base for {path}" ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 1269 - 1285, Add a unit test alongside test_infer_dataset_base_from_version_manifest_path and test_infer_dataset_base_from_latest_manifest_path covering unrecognized paths, including bare, wrong-directory, wrong-extension, and root-level latest manifests, and assert infer_dataset_base_from_manifest_path returns None. Also add test coverage that exercises the indices_error and transaction_error rendering branches at their respective error-formatting sites.
179-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExternal transaction failures lose the attempted path.
Lines 190-194 surface bare
object_store/prostmessages, so a missing or corrupt transaction file renders as e.g.NotFoundwith no indication of which_transactions/<file>was tried. Includepathin each message; extracting a small helper also flattens the four-level match pyramid.♻️ Proposed refactor
- match object_store.inner.get(&path).await { - Ok(result) => match result.bytes().await { - Ok(data) => match pb::Transaction::decode(data) { - Ok(transaction) => OptionalSection::Loaded(TransactionView { - source: TransactionSource::External { path }, - transaction, - }), - Err(e) => OptionalSection::Error(e.to_string()), - }, - Err(e) => OptionalSection::Error(e.to_string()), - }, - Err(e) => OptionalSection::Error(e.to_string()), - } + match read_external_transaction(object_store, &path).await { + Ok(transaction) => OptionalSection::Loaded(TransactionView { + source: TransactionSource::External { path }, + transaction, + }), + Err(e) => OptionalSection::Error(format!( + "failed to read transaction file {}: {}", + path, e + )), + }with:
async fn read_external_transaction( object_store: &ObjectStore, path: &Path, ) -> Result<pb::Transaction> { let data = object_store.inner.get(path).await?.bytes().await?; Ok(pb::Transaction::decode(data)?) }As per coding guidelines: "Include full error context, including variable names, values, sizes, and types."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 179 - 195, Update the external transaction loading flow around TransactionSource::External to include the attempted path in every object-store, byte-reading, and protobuf decode error message. Extract the nested reads and decode into a small read_external_transaction helper if appropriate, then map its errors at the caller while preserving OptionalSection::Loaded, OptionalSection::Error, and the existing path value.Source: Coding guidelines
1183-1187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
FLAG_UNKNOWNmask invariant.
FLAG_UNKNOWN - 1is only the mask of known feature flags becauseFLAG_UNKNOWNis defined as the first bit after all known flags. Add a comment near this calculation so future flag additions preserve the invariant.📝 Proposed change
- let known_mask = FLAG_UNKNOWN - 1; - let unknown = flags & !known_mask; + // FLAG_UNKNOWN is the first bit above all defined feature flags, so FLAG_UNKNOWN - 1 + // masks exactly the known bits. Any bit at or above it is reported as unknown. + let known_mask = FLAG_UNKNOWN - 1; + let unknown = flags & !known_mask;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-tools/src/manifest.rs` around lines 1183 - 1187, Add a concise comment immediately above the known_mask calculation in the manifest flag-processing code, documenting that FLAG_UNKNOWN is the first bit after all known feature flags and that FLAG_UNKNOWN - 1 therefore masks known flags. Ensure future flag additions preserve this ordering invariant.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-table/src/io/manifest.rs`:
- Around line 30-33: Add synchronized, compiling Rustdoc examples to the public
readers read_manifest_proto (rust/lance-table/src/io/manifest.rs:30-33) and
read_manifest (rust/lance-table/src/io/manifest.rs:120-122), using their current
signatures and documenting links to the relevant protobuf and semantic Manifest
types or methods. Ensure both examples remain valid if the APIs change.
- Around line 35-41: Add direct tests for read_manifest_proto covering normal
round-trip decoding to the expected pb::Manifest and a stale known_size value
that successfully retries and returns the correct protobuf. Extend or add
focused manifest tests alongside test_roundtrip_manifest, reusing existing
object-store and manifest setup.
In `@rust/lance-tools/src/lib.rs`:
- Line 5: Change the manifest module declaration to crate visibility by updating
pub mod manifest to pub(crate) mod manifest; in the crate root. Preserve
internal access from the CLI entrypoint through
crate::manifest::show_table_manifest without exposing lance_tools::manifest
publicly.
In `@rust/lance-tools/src/manifest.rs`:
- Around line 1287-1338: Strengthen test_parse_all_test_data_manifests so it
validates rendered manifest values rather than only the manifest:, schema:, and
fragments: headings. For at least one known checked-in manifest, assert a stable
value such as the version, fragment count, or data-file path, or compare the
rendered output against an approved snapshot while preserving the existing
compatibility checks.
- Around line 34-41: Change the visibility of the crate-internal
LanceToolManifest struct from pub to pub(crate). Keep show_table_manifest as the
crate entry point and leave the struct’s fields and behavior unchanged.
- Around line 1105-1115: Rename format_option_usize and format_optional_usize to
explicitly identify their None sentinels, such as unknown- and absent-specific
names, then update callers at the noted locations so unknown values use the
“unknown” formatter and absent values use the “absent” formatter.
---
Other comments:
In `@rust/lance-tools/src/manifest.rs`:
- Around line 608-618: Update the write_kv call in the parse-failure manifest
formatting path to rename the uuid_bytes key to a precise byte-count name such
as uuid_byte_count, matching the existing length value; if retaining diagnostic
detail, add a separate hex representation of the raw UUID bytes rather than
labeling the count as bytes.
- Line 855: Update the manifest rendering in the write_kv call for
update.update_mode to resolve the prost enum discriminator through its generated
accessor or enumerator, emitting the enum variant name for known values and
retaining the numeric discriminator only for unknown values.
---
Nitpick comments:
In `@rust/lance-tools/src/manifest.rs`:
- Around line 111-133: Add doc comments for
infer_dataset_base_from_manifest_path and path_from_parts documenting the
recognized <base>/_latest.manifest and <base>/_versions/*.manifest layouts, that
None means the dataset base cannot be inferred and external transaction files
cannot be resolved, and that root-level manifests return None because
path_from_parts rejects an empty prefix.
- Around line 73-97: In the manifest construction flow, remove the full protobuf
clone and replace retained raw_manifest presence checks with boolean flags
captured from the original raw manifest before conversion. Add has_timestamp and
has_data_format fields to the containing struct, initialize them in this
constructor, and update the timestamp/data-format checks in the relevant methods
to use those flags.
- Around line 1269-1285: Add a unit test alongside
test_infer_dataset_base_from_version_manifest_path and
test_infer_dataset_base_from_latest_manifest_path covering unrecognized paths,
including bare, wrong-directory, wrong-extension, and root-level latest
manifests, and assert infer_dataset_base_from_manifest_path returns None. Also
add test coverage that exercises the indices_error and transaction_error
rendering branches at their respective error-formatting sites.
- Around line 179-195: Update the external transaction loading flow around
TransactionSource::External to include the attempted path in every object-store,
byte-reading, and protobuf decode error message. Extract the nested reads and
decode into a small read_external_transaction helper if appropriate, then map
its errors at the caller while preserving OptionalSection::Loaded,
OptionalSection::Error, and the existing path value.
- Around line 1183-1187: Add a concise comment immediately above the known_mask
calculation in the manifest flag-processing code, documenting that FLAG_UNKNOWN
is the first bit after all known feature flags and that FLAG_UNKNOWN - 1
therefore masks known flags. Ensure future flag additions preserve this ordering
invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8e392696-b046-4c02-aefc-2dea9f9573bb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust/lance-table/src/io/manifest.rsrust/lance-tools/Cargo.tomlrust/lance-tools/README.mdrust/lance-tools/src/cli.rsrust/lance-tools/src/lib.rsrust/lance-tools/src/manifest.rs
|
Hi @Xuanwo @westonpace, sorry for the ping. Could you help take a look at this PR when you have a chance? This adds The targeted checks passed locally:
Thanks! |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
lance-gatekeeper
Bot
removed
the
K-changes
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Both transaction renderer branches now match the current compacted_sstables protobuf contract. The command is additive and read-only, preserves the semantic manifest and index read paths, and I found no remaining decision-relevant risk.
lance-gatekeeper
Bot
added
the
K-approved
This branch has not been deployed
1 participant
feat(tools): add table manifest command
Which issue does this PR close?
None.
What changes are included in this PR?
This PR adds a
lance-tools table manifestcommand for inspecting Lance table manifest files from local or object-store paths.Included changes:
table manifest --source <manifest>CLI entry point tolance-tools.read_manifest_protofromlance-tableso tools can read the raw protobuf manifest before conversion to the semanticManifesttype.lance-tools table manifestcommand in the tool README.test_datamanifests and verifies the rendered output contains the expected top-level sections.Are there any user-facing changes?
Yes. Users can now inspect a table manifest with:
This is an additive CLI feature and does not change existing commands or public dataset behavior.
Testing
git diff --cached --checkTargeted Rust tests were not run locally for this temporary PR description.