Skip to content

docs(linalg): document SIMD conversion panics - #8862

Open
lance-gatefixer[bot] wants to merge 3 commits into
mainfrom
gatekeeper/fix-8861-1
Open

docs(linalg): document SIMD conversion panics#8862
lance-gatefixer[bot] wants to merge 3 commits into
mainfrom
gatekeeper/fix-8861-1

Conversation

@lance-gatefixer

Copy link
Copy Markdown
Contributor

Summary

  • document the minimum slice length for all six public SIMD conversions that panic on short input
  • describe the AVX2 Hamming popcount as the PSHUFB nibble-lookup algorithm it implements

Root cause

The slice length assertions were added without matching rustdoc panic contracts. Separately, the AVX2 implementation was labeled Harley–Seal even though it independently uses PSHUFB nibble lookups and has no carry-save-adder tree.

Validation

  • cargo test -p lance-linalg
  • cargo doc -p lance-linalg --no-deps
  • cargo fmt --all
  • cargo clippy --all --tests --benches -- -D warnings

Fixes #8861

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer documentation Improvements or additions to documentation labels Aug 29, 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 29, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8862 remains blocked at remote head 535b015 by two failing Rust checks that are not attributable to its rustdoc-only repair.

linux-build (job 99091892349) failed after both io_uring workers returned Cannot allocate memory (os error 12); the exact base SHA run failed for the same reason in another io_uring test. mac-build (stable) (job 99091892325) failed the Legacy/stable-row-ID/Normal/IVF-HNSW-SQ test_ann_prefilter case with Vector column vector not found in batch; the exact base macOS job passed, and all 32 focused cases passed locally on this head. No speculative code or workflow change was made; main 108f78e is an ancestor, the worktree is clean, and git diff --check passed.

Smallest action: rerun the two failed Rust jobs on healthy runners. Alternatively, address the inherited io_uring runner-memory failure and investigate the flaky legacy vector-index test independently, then rerun CI.

@LuciferYang

Copy link
Copy Markdown
Contributor

The # Panics sections are on the impl block rather than on fn from, which changes where rustdoc puts them. I rendered both placements to be sure rather than going on convention.

With the section on the impl block, as here, the heading attaches to the impl header and lands outside impl-items, so from's own entry has no documentation at all:

<h3 class="code-header">impl From<&[f32]> for f32x8</h3>
<div class="docblock"><h4 id="panics-1">§Panics</h4>
  <p>Panics if <code>value</code> contains fewer than 8 elements.</p></div>
</section></summary><div class="impl-items">...

With it on the method, it attaches to from:

<section id="method.from" class="method trait-impl">
  <h4 class="code-header">fn from(value: &[f32]) -> Self</h4>
</section></summary><div class="docblock">
  <p>Loads the first 8 elements of <code>value</code>.</p>
  <h5 id="panics-1">§Panics</h5>
  <p>Panics if <code>value</code> has fewer than 8 elements.</p></div>

Both are valid rustdoc and neither warns under -D warnings, so this is not a build problem. It matters because the reader who wants to know whether from can panic expands from, and in the first form that entry is empty. The heading level also shifts from h5 to h4, which puts a "Panics" heading at the same visual level as the trait-impl group headers on the struct page.

Two smaller things while you are in there. A bare # Panics with no summary line reads oddly in the rendered output, so a one-liner above it is worth adding: "Loads the first N elements of value" is accurate for all six, since the assert is >= and the body is a prefix load with no reordering. And the asserts these document say "values" ("f32x8 requires at least 8 values, got {}"), while the new text says "elements", so the doc and the panic message a caller actually sees use different words for the same thing. Either is fine, but picking one avoids the reader wondering whether they differ.

The hamming.rs line is right, and it is a real correction rather than a rewording: the body is a nibble lookup through _mm256_shuffle_epi8 plus _mm256_sad_epu8, with no carry-save-adder tree anywhere, so "Harley-Seal" named an algorithm the function does not implement. Worth saying in the description, because that half of the diff is not about panics and the title does not cover it.

I have the same change with the method placement, the summary lines, and the six impls on a branch, from filing #8861. Happy to push it as an alternative if that is easier than iterating here, or to leave this one to pick up the notes.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in b6af832: moved all six conversion docs onto fn from, added prefix-load summaries, and aligned “values” with the panic messages. Rendered rustdoc now places each summary and Panics section under method.from; the PR description already separately calls out the AVX2 Hamming correction.

@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 29, 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 29, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8862 remains blocked at remote head b6af832 by an inherited linux-build io_uring runner-memory failure.

linux-build job 99094596632 failed uring::tests::test_read_small_file and test_read_range after both io_uring workers returned Cannot allocate memory (os error 12). The exact current base SHA 108f78e failed its own linux-build job 99033341209 with the identical two-worker error in test_read_range; this PR changes only five lance-linalg rustdoc files, while current-head format, clippy, rustdoc, Linux ARM, and Windows checks pass. Both refs and logs were refreshed, the base tip is an ancestor of the remote head, the worktree is clean, and git diff --check passes, so no speculative source or workflow change was made.

Smallest action: rerun the failed linux-build job on a healthy runner with sufficient resources. Alternatively, correct the shared runner io_uring memory/resource limit and rerun CI.

@LuciferYang

Copy link
Copy Markdown
Contributor

I wrote the same change independently in #8865 and am closing mine in favour of this one, which came first. Two things I checked on the way that support it.

The Harley-Seal name really is wrong, not just imprecise: the body is a _mm256_shuffle_epi8 nibble lookup followed by _mm256_sad_epu8, with no carry-save-adder tree anywhere, so the only thing it shares with Harley-Seal is being a popcount. After this rename a grep for "Harley" across the crate returns nothing, so the name is gone rather than moved.

The six impls here are the right six. The From<&[T; N]> array impls take a fixed-size reference and cannot be short, so they need no # Panics, and adding one to them would be the easy over-correction.

@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.

Merging the updated base leaves the reviewed patch unchanged. The six method-level panic contracts still render under fn from with the correct limits, and the AVX2 Hamming description remains an accurate match for its PSHUFB nibble lookup.

@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.

bug: lance-linalg SIMD docs omit the panics and name the wrong popcount algorithm

1 participant