mirror of
https://github.com/lancedb/lancedb.git
synced 2026-08-18 12:08:35 +00:00
Justin Miller
c7ea91f3ea
## Description `Table::optimize()` compacts through `lance::dataset::optimize::compact_files` (`rust/lancedb/src/table/optimize.rs:155`). Until lance-format/lance#7965 that rewrite corrupted blob columns holding null or empty values, which is what #3744 reports: - **storage 2.0** (legacy v1 `lance-encoding:blob` descriptors): every payload following a null or empty row in the same fragment was rewritten as `{position: 0, size: 0}`, so it read back as `b""` and the new fragment no longer referenced the bytes — silent payload loss, unrecoverable once the pre-optimize versions are pruned. - **storage 2.2** (blob v2): a valid empty value was rewritten as null, destroying the null-vs-empty distinction. Both manifestations share one root cause: `is_inline_null_blob` classified any inline blob with `position == 0 && size == 0` as null, which is also exactly what a *valid empty value* looks like. Such rows were dropped from `blob_read_addrs`, misaligning every payload that followed. The behaviour is already correct on `main`: the vendored lance crate first carried the fix at `v10.0.0-beta.3` (#3710) and is now `v10.1.0-beta.1` (#3757). What was missing is coverage — nothing in this repo exercised a blob column containing a null or empty value through `optimize()`, which is why this shipped unnoticed. This PR adds that guard. ## Tests Two tests in `rust/lancedb/tests/blob_integration.rs`, reusing the file's existing 64 KiB dedicated-blob helpers and a delete-triggered fragment rewrite. After `id IN (1, 4)` is deleted the surviving rows are `2` (null), `3` (valid empty), `5` and `6` (payloads) — payloads sit immediately after the null/empty, which is where the misalignment landed. - `optimize_preserves_v1_blob_payloads_with_null_and_empty` — storage 2.0; asserts the **payload bytes** are unchanged across `OptimizeAction::All` (what the Python/Node `optimize()` bindings invoke). Payloads are read through `lance::Dataset::take_blobs`, since `Table::fetch_blobs` rejects legacy v1 columns. The before/after descriptors are reported on failure but deliberately *not* asserted: compaction repacks the blob file, so they shift legitimately (id 5 `(131072, 65536)` → `(0, 65536)`, id 6 `(196608, 65536)` → `(65536, 65536)`). Note that a post-compaction `position: 0` is both the legitimate first-payload offset and the bug's signature, so asserting descriptors would be actively misleading. - `optimize_preserves_blob_v2_null_and_empty_distinction` — storage >= 2.2; asserts a null stays null and a valid empty value stays non-null empty. Both assert the pre-optimize state first, so a setup change that stops producing the null/empty/payload mix fails loudly instead of passing vacuously. Both also assert the returned `CompactionMetrics` show a fragment was actually rewritten. These tests depend on `delete("id IN (1, 4)")` pushing the fragment past lance's `materialize_deletions_threshold` (0.1 by default; 2 of 6 rows here). That coupling is invisible and unasserted otherwise: against a forced no-op (`materialize_deletions_threshold: 1.5`) the metrics come back all zeroes and *every payload assertion still passes*. Since the whole point of these tests is to survive dependency changes, they check that the rewrite happened rather than trusting the planner to keep selecting the fragment. Guard verified against a pre-fix lance: with the published `lancedb==0.36.0` wheel (vendors lance 9.0.0), `Table.optimize()` on the same data rewrites the descriptors of the two rows following the null/empty from `(131072, 65536)` and `(196608, 65536)` to `(0, 0)`, and the payloads read back empty. Against the pinned `v10.1.0-beta.1`, all 39 tests in the file pass, adding roughly 10–20 ms to the file's runtime. ## Not addressed here - **No released artifact has the fix yet.** PyPI `lancedb` 0.36.0 (2026-07-29) vendors lance 9.0.0; npm `@lancedb/lancedb` 0.37.1-beta.0 predates the bump. No 9.x lance tag carries the fix: `v10.0.0-beta.3` is the first tag containing it, every `v9.1.0-beta.1`…`beta.8` is behind it, and `v9.0.0` / `v9.0.1-rc.1` sit on a diverged branch without it. A stable lancedb release needs a stable lance >= 10. - **The version skew #3744 flagged is still live.** `python/pyproject.toml` pins `pylance==9.0.0rc1` for the `tests` extra against a vendored `10.1.0-beta.1`, so Python CI still cannot observe this class of divergence. - **Only the single-fragment rewrite shape is covered.** Both tests rewrite one fragment by materializing deletions. lance's own `test_compact_blob_v1/v2_preserves_null_empty_and_payload_order` cover the multi-fragment merge shape (3 fragments → 1) at unit level, so this PR is complementary rather than redundant — it covers the binding-level path through `Table::optimize` — but it would not catch a regression that only appears when *merging* fragments. `multi_fragment_dedicated_blob_table` in the same file makes that a cheap follow-up. Closes #3744 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>