feat(tools): add table manifest command#7970
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
| /// Read the raw Manifest protobuf from a URI. | ||
| /// | ||
| /// This only reads manifest files. It does not read data files. | ||
| /// This only reads manifest files. It does not read data files or translate the | ||
| /// protobuf into the semantic [`Manifest`] type. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document both public manifest readers with synchronized examples.
rust/lance-table/src/io/manifest.rs#L30-L33: add a compiling example forread_manifest_protoand link its protobuf/semantic output types.rust/lance-table/src/io/manifest.rs#L120-L122: add a compiling example forread_manifestand link relevant types or methods.
As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
📍 Affects 1 file
rust/lance-table/src/io/manifest.rs#L30-L33(this comment)rust/lance-table/src/io/manifest.rs#L120-L122
🤖 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-table/src/io/manifest.rs` around lines 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.
Source: Coding guidelines
| pub async fn read_manifest_proto( | ||
| object_store: &ObjectStore, | ||
| path: &Path, | ||
| known_size: Option<u64>, | ||
| ) -> Result<Manifest> { | ||
| ) -> Result<pb::Manifest> { | ||
| let buf = read_manifest_bytes(object_store, path, known_size).await?; | ||
| Ok(pb::Manifest::decode(buf)?) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct coverage for the new raw-protobuf API.
test_roundtrip_manifest only verifies read_manifest; add tests that assert read_manifest_proto returns the expected protobuf and that a stale known_size retries successfully.
As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 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-table/src/io/manifest.rs` around lines 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.
Source: Coding guidelines
| // SPDX-FileCopyrightText: Copyright The Lance Authors | ||
|
|
||
| pub mod cli; | ||
| pub mod manifest; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)lib\.rs$|(^|/)cli\.rs$|(^|/)manifest\.rs$|Cargo\.toml$'
echo
echo "lib.rs:"
sed -n '1,80p' rust/lance-tools/src/lib.rs
echo
echo "manifest.rs outline:"
ast-grep outline rust/lance-tools/src/manifest.rs --view compact || true
echo
echo "manifest.rs relevant entries:"
rg -n "^(pub )?|show_table_manifest|TableManifest|struct |enum " rust/lance-tools/src/manifest.rs
echo
echo "cli.rs manifest usages:"
rg -n "crate::manifest|manifest::|show_table_manifest|lance_tools::manifest" rust/lance-tools/src/cli.rs
echo
echo "workspace/crate config relevant:"
sed -n '1,160p' rust/lance-tools/Cargo.tomlRepository: lance-format/lance
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "lance-tools package metadata:"
python3 - <<'PY'
import configparser, re, json
p='rust/lance-tools/Cargo.toml'
text=open(p).read()
print(text)
PY
echo
echo "manifest struct visibility and internal call path:"
rg -n -C 3 '^(pub )?(struct LanceToolManifest|enum OptionalSection|struct IndexView|struct TransactionView|struct TransactionSource|struct ManifestSource|async fn open|impl LanceToolManifest|pub\(crate\) async fn show_table_manifest)' rust/lance-tools/src/manifest.rs
rg -n -C 2 'LanceTableManifestArgs|show_table_manifest|crate::manifest::manifest_path|manifest::show_table_manifest' rust/lance-tools/src/cli.rs rust/lance-tools/src/*.rs
echo
echo "all external references to lance_tools::manifest / crate::manifest outside lance-tools:"
rg -n 'lance_tools::manifest|lance-tools::manifest|lance_tools::util|crate::manifest|manifest::show_table_manifest' --glob '!rust/lance-tools/**' .
echo
echo "all references to public types in manifest.rs (excluding tests/internal helpers if apparent):"
rg -n 'LanceToolManifest|OptionalSection|IndexView|TransactionView|TransactionSource|ManifestSource' rust/lance-tools/src/manifest.rsRepository: lance-format/lance
Length of output: 5693
Keep the manifest module crate-private.
manifest is only reached from lance-tools CLI (crate::manifest::show_table_manifest), and the CLI entrypoint itself is pub(crate). Use pub(crate) mod manifest; unless lance_tools::manifest is intended as public API.
🤖 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/lib.rs` at 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.
Source: Coding guidelines
| pub struct LanceToolManifest { | ||
| path: Path, | ||
| dataset_base: Option<Path>, | ||
| raw_manifest: pb::Manifest, | ||
| manifest: Manifest, | ||
| indices: OptionalSection<Vec<IndexView>>, | ||
| transaction: OptionalSection<TransactionView>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Prefer pub(crate) for this crate-internal type.
LanceToolManifest is only constructed and rendered within this module via the pub(crate) show_table_manifest entry point; exposing it as pub widens the crate's public API surface without a doc comment.
♻️ Proposed change
-pub struct LanceToolManifest {
+pub(crate) struct LanceToolManifest {As per coding guidelines: "Prefer pub(crate) over pub for crate-internal items, and use pub use re-exports for the actual public API surface."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub struct LanceToolManifest { | |
| path: Path, | |
| dataset_base: Option<Path>, | |
| raw_manifest: pb::Manifest, | |
| manifest: Manifest, | |
| indices: OptionalSection<Vec<IndexView>>, | |
| transaction: OptionalSection<TransactionView>, | |
| } | |
| pub(crate) struct LanceToolManifest { | |
| path: Path, | |
| dataset_base: Option<Path>, | |
| raw_manifest: pb::Manifest, | |
| manifest: Manifest, | |
| indices: OptionalSection<Vec<IndexView>>, | |
| transaction: OptionalSection<TransactionView>, | |
| } |
🤖 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 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.
Source: Coding guidelines
| fn format_option_usize(value: Option<usize>) -> String { | ||
| value | ||
| .map(|value| value.to_string()) | ||
| .unwrap_or_else(|| "unknown".to_string()) | ||
| } | ||
|
|
||
| fn format_optional_usize(value: Option<usize>) -> String { | ||
| value | ||
| .map(|value| value.to_string()) | ||
| .unwrap_or_else(|| "absent".to_string()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
format_option_usize and format_optional_usize are indistinguishable by name.
The two differ only in the sentinel emitted for None ("unknown" vs "absent"), and neither name says which. A caller picking the wrong one silently mislabels absent data. Encode the sentinel in the name (or pass it as an argument).
♻️ Proposed change
-fn format_option_usize(value: Option<usize>) -> String {
- value
- .map(|value| value.to_string())
- .unwrap_or_else(|| "unknown".to_string())
-}
-
-fn format_optional_usize(value: Option<usize>) -> String {
- value
- .map(|value| value.to_string())
- .unwrap_or_else(|| "absent".to_string())
-}
+/// Formats a count that the manifest may not record; `None` renders as `unknown`.
+fn format_option_usize_or_unknown(value: Option<usize>) -> String {
+ value
+ .map(|value| value.to_string())
+ .unwrap_or_else(|| "unknown".to_string())
+}
+
+/// Formats an optional field; `None` renders as `absent`.
+fn format_option_usize_or_absent(value: Option<usize>) -> String {
+ value
+ .map(|value| value.to_string())
+ .unwrap_or_else(|| "absent".to_string())
+}Callers to update: Lines 403, 409, 535 (unknown) and Lines 293, 305 (absent).
As per coding guidelines: "Use precise names describing what values are."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn format_option_usize(value: Option<usize>) -> String { | |
| value | |
| .map(|value| value.to_string()) | |
| .unwrap_or_else(|| "unknown".to_string()) | |
| } | |
| fn format_optional_usize(value: Option<usize>) -> String { | |
| value | |
| .map(|value| value.to_string()) | |
| .unwrap_or_else(|| "absent".to_string()) | |
| } | |
| /// Formats a count that the manifest may not record; `None` renders as `unknown`. | |
| fn format_option_usize_or_unknown(value: Option<usize>) -> String { | |
| value | |
| .map(|value| value.to_string()) | |
| .unwrap_or_else(|| "unknown".to_string()) | |
| } | |
| /// Formats an optional field; `None` renders as `absent`. | |
| fn format_option_usize_or_absent(value: Option<usize>) -> String { | |
| value | |
| .map(|value| value.to_string()) | |
| .unwrap_or_else(|| "absent".to_string()) | |
| } |
🤖 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 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.
Source: Coding guidelines
| #[tokio::test] | ||
| async fn test_parse_all_test_data_manifests() { | ||
| let test_data_dir = repo_root().join("test_data"); | ||
| let mut manifest_paths = Vec::new(); | ||
| collect_manifest_paths(&test_data_dir, &mut manifest_paths); | ||
| manifest_paths.sort(); | ||
|
|
||
| assert!( | ||
| !manifest_paths.is_empty(), | ||
| "test_data should contain manifest compatibility samples" | ||
| ); | ||
|
|
||
| for manifest_path in manifest_paths { | ||
| let args = LanceTableManifestArgs { | ||
| source: manifest_path.to_string_lossy().to_string(), | ||
| }; | ||
| let manifest = LanceToolManifest::open(&args) | ||
| .await | ||
| .unwrap_or_else(|e| panic!("failed to parse {}: {}", manifest_path.display(), e)); | ||
| if let OptionalSection::Error(error) = &manifest.indices { | ||
| panic!( | ||
| "failed to parse index section for {}: {}", | ||
| manifest_path.display(), | ||
| error | ||
| ); | ||
| } | ||
| if let OptionalSection::Error(error) = &manifest.transaction { | ||
| panic!( | ||
| "failed to parse transaction for {}: {}", | ||
| manifest_path.display(), | ||
| error | ||
| ); | ||
| } | ||
|
|
||
| let rendered = manifest.to_string(); | ||
| assert!( | ||
| rendered.contains("manifest:"), | ||
| "rendered output missing manifest section for {}", | ||
| manifest_path.display() | ||
| ); | ||
| assert!( | ||
| rendered.contains("schema:"), | ||
| "rendered output missing schema section for {}", | ||
| manifest_path.display() | ||
| ); | ||
| assert!( | ||
| rendered.contains("fragments:"), | ||
| "rendered output missing fragments section for {}", | ||
| manifest_path.display() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assertions do not actually verify the rendered report.
Checking for the literal headings manifest:, schema:, and fragments: passes even if every field value is wrong or every optional section degrades to absent — the test effectively only proves open() and Display did not panic. Since the report format is the feature, add at least one value-level assertion against a known checked-in manifest (version, fragment count, a data file path), or a snapshot comparison so format regressions surface.
As per coding guidelines: "Every bugfix and feature must have corresponding tests."
🤖 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 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.
Source: Coding guidelines
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.