chore(deps): bump lance to 9.1.0-beta.3 - #58
Conversation
Upgrade the pinned Lance revision from e0e977a6 (7.0.0-beta.7) to e934cc2c (9.1.0-beta.3), the core baseline used by the distributed index build track (lance-format#55). - lance-core error variants gained an implicit backtrace field, so direct struct construction no longer compiles. Migrate all FFI validation errors to the new Error::invalid_input* / index_not_found constructors. Error variants and messages are unchanged. - Lance 9.1 classifies SQL parser failures and unknown expression columns as invalid user input instead of internal errors; update the three affected error-code assertions in c_api_test.rs. - datafusion dev-dependency 53 -> 54 to match the pinned Lance.
Malformed SQL and unknown expression columns now surface as LANCE_ERR_INVALID_ARGUMENT (previously LANCE_ERR_INTERNAL) in lance_dataset_delete and lance_dataset_add_columns_sql.
![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 9.1 flipped WriteParams::auto_cleanup from Some(default) to None. Since lance-c exposes neither cleanup configuration nor an explicit cleanup operation, datasets created through the C writer would silently lose their only C-visible reclamation path. Set auto_cleanup explicitly to the pre-9.1 default (every 20 versions, older than 14 days) so C-visible behavior is unchanged by the upgrade. Addresses the gatekeeper review on lance-format#58.
lance-gatekeeper
Bot
removed
the
K-changes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
There was a problem hiding this comment.
Ran it locally against e934cc2c — check, clippy -D warnings, and 257 tests all pass. But please fix the format
Also diffed WriteParams::default() and CompactionOptions::default() across 7.0 → 9.1: auto_cleanup is the only changed default, and the config upsert is Create-only, so append/overwrite on existing datasets are unaffected. Audit looks complete.
One I can't anchor to: tests/c_api_test.rs:4947 still says delete "surfaces these as Internal" — no longer true after this PR.
| // accumulate versions for datasets created through the C writer. | ||
| // Preserve the pre-9.1 default (every 20 versions, older than 14 | ||
| // days) to keep C-visible behavior unchanged across the upgrade. | ||
| auto_cleanup: Some(AutoCleanupParams::default()), |
There was a problem hiding this comment.
The comment promises the pre-9.1 policy, but the code takes whatever upstream's Default becomes. Worth spelling out:
auto_cleanup: Some(AutoCleanupParams { interval: 20, older_than: TimeDelta::days(14) }),Also, lance.h never mentions auto-cleanup — C callers of lance_dataset_versions / restore won't know 14-day-old versions get reclaimed. Worth a line there.
There was a problem hiding this comment.
Fixed in d8e3ba7: the writer now pins AutoCleanupParams { interval: 20, older_than: TimeDelta::days(14) } explicitly, and lance_dataset_write's doc in lance.h now states the auto-cleanup policy for lance_dataset_versions / lance_dataset_restore callers.
| assert!( | ||
| config.contains_key("lance.auto_cleanup.older_than"), | ||
| "auto-cleanup older_than must be recorded in the manifest config" | ||
| ); |
There was a problem hiding this comment.
Checks the key exists but not the value, unlike the interval assert above. insert.rs formats with humantime, so:
| assert!( | |
| config.contains_key("lance.auto_cleanup.older_than"), | |
| "auto-cleanup older_than must be recorded in the manifest config" | |
| ); | |
| assert_eq!( | |
| config.get("lance.auto_cleanup.older_than").map(String::as_str), | |
| Some("14days"), | |
| "auto-cleanup older_than must be recorded in the manifest config" | |
| ); |
There was a problem hiding this comment.
Fixed in d8e3ba7: the test now asserts the recorded value Some("14days"), matching the interval assertion (verified against insert.rs's humantime formatting).
| location: snafu::location!(), | ||
| })?; | ||
| // NULL is rejected above; only the empty case reaches here. | ||
| .ok_or_else(|| lance_core::Error::invalid_input("predicate must not be empty"))?; |
There was a problem hiding this comment.
Nit: this and writer.rs:218 use invalid_input(...); the other 94 sites use invalid_input_source(...). Worth picking one.
There was a problem hiding this comment.
Leaving as-is: both constructors produce the same InvalidInput variant, and invalid_input(msg) is upstream's purpose-built constructor for message-only errors — the 94 invalid_input_source("...".into()) sites are the awkward ones. Unifying those is a codebase-wide cleanup that belongs in its own PR, not this dependency bump.
- writer.rs: construct AutoCleanupParams { interval: 20, older_than:
14 days } explicitly instead of inheriting AutoCleanupParams::default(),
so a future upstream default change cannot silently shift the C-visible
storage-lifecycle contract (adds a direct chrono edge to name TimeDelta;
already in the graph via lance, no new packages).
- lance.h: document the auto-cleanup policy on lance_dataset_write so C
callers of lance_dataset_versions / lance_dataset_restore know versions
older than 14 days are reclaimed.
- c_api_test: assert the recorded older_than value ("14days") rather than
mere key presence, matching the interval assertion.
- c_api_test: drop the stale "delete surfaces parser errors as Internal"
note — Lance 9.1 classifies them as InvalidArgument, as
test_delete_invalid_predicate_rejected asserts.
- cargo fmt the auto-cleanup regression test.
|
Addressed the remaining review items in d8e3ba7:
|
lance-gatekeeper
Bot
removed
the
K-approved
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The cleanup contract is now fully pinned at the C wrapper boundary: the policy uses explicit 20-version/14-day values, the public header documents the time-travel horizon, and the regression test verifies both persisted manifest values. The formatting and stale error-classification comment are also corrected.
lance-gatekeeper
Bot
added
the
K-approved
|
Hi @jja725 — friendly ping 🙂 All review comments are addressed in d8e3ba7 (explicit The two workflows on the new head are awaiting maintainer approval — could you approve the CI runs when you get a chance? Thanks! |
lance-gatekeeper
Bot
added
K-approved
## Summary Implements PR 1 from #55: the foundational E1 + E2 primitives for distributed index builds. - adds snapshot-owned, single-use scalar and vector segment builders - builds over explicit fragment subsets without committing a dataset manifest - supports caller-assigned segment UUIDs where Lance core permits them - trains reusable shared IVF centroids and residual PQ codebooks - injects shared models through the Arrow C Data Interface - serializes and parses protobuf `IndexMetadata` with C getters - adds move-only C++ RAII wrappers for builders, models, metadata, and returned bytes ## Design notes - Builds on Lance `9.1.0-beta.3` (`e934cc2c`), the core baseline used by #55; the pin upgrade and its error-constructor compatibility migration landed separately in #58. - Uses a fixed-width `mode` field (`AUTO`, `LOCAL_TRAIN`, `PRECOMPUTED`) instead of a boolean so zero-initialized C options have unambiguous defaults. - Model inputs are synchronously borrowed and restored, allowing one trained IVF/PQ model to build multiple disjoint segments. - Trainer outputs carry metric, dimension, PQ parameters, and IVF identity provenance; builders reject mismatched models. - Arrow schema/array trees are validated before import, including malformed UTF-8, child/buffer structure, slices, NULL values, and overflow-prone dimensions. - Segment metadata bytes are allocated with `malloc` and released with `lance_free_bytes`. ## Impact C and C++ workers can now train a shared model once, build physical index segments over disjoint fragment assignments, and ship protobuf metadata to a coordinator without changing the dataset manifest. Commit-existing-segments remains the next PR in the tracker. ## Validation - `cargo check --all-targets --locked` - `cargo clippy --all-targets --locked -- -D warnings` - `cargo test --locked` (268 C API tests, 3 model tests, 1 helper test) - strict C11 and C++17 compilation with Clang/GCC and `-Werror` - `cargo test --locked --test compile_and_run_test -- --ignored --test-threads=1` - real C and C++ dynamic-link runs - IVF training -> residual PQ training -> reuse the same models for two disjoint fragment segments Tracks #55.
2 participants
Summary
Upgrade the pinned Lance revision from
e0e977a6(7.0.0-beta.7) toe934cc2c(9.1.0-beta.3), the core baseline used by the distributed index build track (#55). Split out of #57 so the engine upgrade lands (and can be reverted) independently of the feature work.What the upgrade forced
backtracefield, so direct struct construction (Error::InvalidInput { source, location }) no longer compiles. All FFI validation errors migrate to the newError::invalid_input/invalid_input_source/index_not_foundconstructors. Variants and user-facing messages are unchanged; location (and now backtrace) are captured implicitly by snafu.InvalidArgument) instead ofInternal. The affected tests assert the new classification.WriteParams::auto_cleanupfromSome(default)toNone. Because lance-c exposes neither cleanup configuration nor an explicit cleanup operation, that flip would silently remove the only C-visible reclamation path for datasets created through the C writer. The wrapper now setsauto_cleanupexplicitly to the pre-9.1 default (every 20 versions, older than 14 days), so C-visible behavior is unchanged. Covered bytest_write_preserves_auto_cleanup_default. A C cleanup API (lance_dataset_cleanup_old_versions, optionalLanceWriteParamsknobs) can be proposed as its own PR.No public API, ABI, or behavior changes beyond the three reclassified error codes above.
Validation
cargo check --all-targetscargo clippy --all-targets -- -D warningscargo test(257 tests)cargo test --test compile_and_run_test -- --ignored --test-threads=1(real C and C++ dynamic-link runs)Tracks #55. The distributed index segment build PR (#57) is stacked on this change.
Notes for reviewers
deletemalformed SQL / unknown column,add_columns_sqlunknown column:LANCE_ERR_INTERNAL→LANCE_ERR_INVALID_ARGUMENT) are behavior changes visible to C callers; the public docs inlance.h/lance.hppare updated accordingly. Abreaking-changelabel may be warranted — maintainer's call.