Skip to content

fix(compaction): reject unsafe binary column layouts - #8510

Merged
Xuanwo merged 7 commits into
mainfrom
gatekeeper/fix-8501-1
Sep 1, 2026
Merged

fix(compaction): reject unsafe binary column layouts#8510
Xuanwo merged 7 commits into
mainfrom
gatekeeper/fix-8501-1

Conversation

@lance-gatefixer

Copy link
Copy Markdown
Contributor

Summary

  • require binary-copy inputs to use the physical column mapping derived from the dataset schema
  • let TryBinaryCopy safely fall back to re-encoding when files use another physical column order
  • add a Decimal128/UInt64 regression covering two uniformly reordered fragments and value round-tripping

Root cause

Binary-copy eligibility only required source files to agree with one another. The copy loop preserved their physical page order, but output metadata was regenerated in dataset-schema order, so uniformly reordered source files crossed logical columns after compaction.

Related work

PR #8479 proposes the same eligibility invariant for the separate metadata-only schema-evolution scenario in #8281. This repair is tied to the direct public-API reordered-column reproduction in #8501 and adds regression coverage for that distinct trigger.

Validation

  • cargo test -p lance dataset::optimize::tests::binary_copy (19 passed)
  • cargo fmt --all -- --check
  • cargo clippy --all --tests --benches -- -D warnings

Fixes #8501

@github-actions github-actions Bot added the bug Something isn't working label Aug 12, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 12, 2026
# Conflicts:
#	rust/lance/src/dataset/optimize.rs
#	rust/lance/src/dataset/optimize/tests/binary_copy.rs
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 31, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 31, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 31, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 31, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8510 at remote head 078a4b259403aaab7504493171caa1ea8796a9ab remains blocked by the mac-build (stable) Check (Actions job 99524897525), where inherited test index::vector::ivf::v2::tests::test_legacy_ivf_pq_cosine_multivec_smoke produced recall 0.49, below its 0.50 requirement.

The current main tip 987adde0d611ada5e5d8176264bf61b388600963 is an ancestor of this head; the effective PR diff is limited to binary-copy compaction regression coverage, and the exact failing test passes locally (1 passed in 0.43s), so no repair-attributable change is supported by the evidence. Rerun the failed mac-build (stable) Check to continue; if it fails again, investigate or stabilize that inherited vector-recall test separately, or provide a repeatable current-head failure.

# Conflicts:
#	rust/lance/src/dataset/optimize/tests/binary_copy.rs
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 1, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 1, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8510 at remote head dd7758940c19575f3707be66ee17c10f59eda595 remains blocked by the failed Python windows Check (Actions job 99906513642), where test_torch_index_with_nans[V3] ended in a PyTorch Inductor PermissionError opening a generated Windows runner temp module after 1,391 tests passed.

The current main tip 7bf7e97f65b06db16058953a71c920e2241ad7c3 is an ancestor of this head; I inspected the live job log and confirmed the effective PR diff is limited to Rust binary-copy compaction code and regression coverage, so the evidence supports no repair-attributable change. Rerun the failed windows Check to continue; if it fails again, investigate the Windows runner/PyTorch temp-module permissions separately, or provide a repeatable current-head failure tied to this repair.

@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 1, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

The latest base only optimizes manifest column-index validation; this PR still applies the version-specific canonical-mapping guard before the restored binary-copy implementation. Noncanonical inputs safely re-encode under TryBinaryCopy, while canonical nested and packed layouts remain eligible across V2.0–V2.3. With PR #8660 reverted by PR #8926, retaining this conservative boundary here remains correct.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 1, 2026
@Xuanwo
Xuanwo merged commit c4086ca into main Sep 1, 2026
36 checks passed
@Xuanwo
Xuanwo deleted the gatekeeper/fix-8501-1 branch September 1, 2026 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Binary-copy compaction corrupts files whose physical column order differs from the schema

1 participant