Skip to content

docs(linalg): document the length contract on the public distance entry points - #8879

Open
LuciferYang wants to merge 6 commits into
lance-format:mainfrom
LuciferYang:docs/linalg-length-contract
Open

docs(linalg): document the length contract on the public distance entry points#8879
LuciferYang wants to merge 6 commits into
lance-format:mainfrom
LuciferYang:docs/linalg-length-contract

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What this changes

Eighteen public items in lance-linalg's distance module gain a # Panics section. All of them already panic; none of them said so.

assert_equal_lengths and assert_batch_layout are always-on asserts, promoted in #8593, #8594 and #8639 precisely so they fire in release. Every public entry point that reaches one now documents it, so cargo doc shows the contract where the caller looks rather than only in the source of a private helper. The two _arrow_batch functions already had a section and are untouched.

The wording splits three ways, and the split is deliberate.

Concrete signatures state the panic directly: l2_f32, l2_distance, l2_distance_uint_scalar, l2_scalar, dot_f32, dot_u8, dot_u8_scalar, l2_u8, l2_u8_scalar.

Generic forwarders point at the trait method instead of repeating it, since what they do depends on the T a caller supplies: l2, dot, dot_distance, l2_distance_batch, dot_distance_batch.

The two trait methods L2::l2 and Dot::dot state a requirement on implementors rather than a fact. Both traits are public and unsealed, so a downstream impl L2 for MyType could zip to the shorter slice and a flat "panics if the lengths differ" would be a claim about code this crate does not control. The five impls in the crate do panic, and the section says so.

L2::l2_batch and Dot::dot_batch spell out all three conditions assert_batch_layout checks, since a reader cannot see them from the signature: non-zero dimension, x.len() equal to it, and a batch length that is a whole multiple.

L2Prepared is the one place where the contract is enforced by debug_assert_eq! rather than by those helpers, so its section says which panic is unconditional and which needs debug assertions, instead of implying it validates as much as l2_distance_batch does.

Test plan

No behavior changes, so the checks are the documentation ones:

  • RUSTDOCFLAGS="-D warnings -D rustdoc::broken_intra_doc_links" cargo doc -p lance-linalg --no-deps: clean, so every [L2::l2]-style reference resolves
  • cargo test -p lance-linalg: 394 passed, 1 ignored
  • cargo fmt --all -- --check and cargo clippy -p lance-linalg --all-targets -- -D warnings: clean
  • cargo clippy -p lance-linalg --lib -- -W clippy::missing_panics_doc reports nothing in these four files. That lint only sees panics in a function's own body, so it confirms the direct-assert layer; the delegating chains were traced by hand.

Verified against the code rather than assumed, since a # Panics section that overstates is worse than none:

  • l2_scalar panics twice, and the length assert runs before chunks_exact(LANES), which rejects a zero chunk size even on an empty slice. Both are documented, in that order.
  • L2Prepared::new panics on dimension == 0 in every profile, since integer division by zero always traps. The remainder case is reported only with debug assertions, which for this repo means dev, test and ci: [profile.ci] inherits dev, and the debug-assertions = false in [profile.ci.package."*"] applies to dependencies, not to workspace members.
  • Every one of the five impl L2 and five impl Dot blocks calls assert_equal_lengths as the first statement of its l2/dot, ahead of the fp16 FFI calls and the x86 runtime tiers, and the only batch override in either family, f32, calls assert_batch_layout before all of its cfg branches.
  • Both asserts are eager. l2_batch is a plain function returning impl Iterator, so it panics at the call rather than at the first next(), which is what makes "panics" the right word for a caller that never polls.

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer documentation Improvements or additions to documentation labels Aug 30, 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 documentation now exposes the established length and batch-layout contracts at the relevant public entry points while preserving the distinction between built-in and downstream trait implementations. The profile-specific L2Prepared cases also match the current behavior.

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

Copy link
Copy Markdown
Contributor Author

The red linux-arm job is lance-indexs scalar::json::tests::test_json_btree_update_uses_trained_target_type::case_2_integer_delta_for_float_index, failing with Not enough memory to continue external sort. Consider increasing the memory limit config at rust/lance-index/src/scalar/json.rs:1278. That is the DataFusion external-sort exhaustion family tracked in #8789, which happens under parallel test load when the memory pool fair share shrinks.

This PR changes only doc comments in rust/lance-linalg/src/distance/, so it compiles no differently and cannot reach a JSON BTree sort.

@LuciferYang

Copy link
Copy Markdown
Contributor Author

A final pass over the rendered docs rather than the diff found one real inconsistency, now fixed: the summary line I added to l2 said "L2 distance" while l2_u8 two files over says "squared L2 distance", and squared is what the function returns. l2_scalar accumulates diff * diff and there is no sqrt anywhere in the file, so a caller who read the new line could compare the result against a radius directly and be wrong.

I only changed the line this PR adds. L2::l2, l2_f32 and l2_distance carry the same imprecision and predate this branch, so straightening those out is a separate change rather than a drive-by here.

The rest of that pass came back clean: all 13 # Panics sections render as real sections with their own anchors and sidebar entries rather than being folded into the paragraph above, every intra-doc link resolves under -D rustdoc::broken_intra_doc_links, and the five forwarders that point at a trait method still line up with what that method documents, including the cases where the forwarder calls its parameters from and to while the target calls them x and y.

@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 30, 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 author’s latest documentation correction changes the new l2 summary to “squared L2 distance,” which matches the implementation’s sum-of-squared-differences semantics. The existing length and batch-layout panic documentation remains accurate, including its trait-implementation and profile-specific L2Prepared distinctions.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer documentation Improvements or additions to documentation K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant