Skip to content

fix(compute): handle pure-Go comparison output offsets#956

Open
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/comparison-offset-bounds
Open

fix(compute): handle pure-Go comparison output offsets#956
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/comparison-offset-bounds

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Rationale for this change

The pure-Go primitive comparison path assumes that a partial output byte always has enough input values to fill it. It also calculates the batch count before consuming that prefix. Short inputs can panic, while longer inputs can process the wrong ranges when the output starts at a non-byte-aligned offset.

This path is used by noasm builds and when the SIMD implementations are unavailable.

What changes are included in this PR?

  • Clamp the prefix to the available value count.
  • Recalculate batches after consuming the prefix.
  • Pass exactly one batch to the array-array operation.
  • Cover offsets 0 through 7 and lengths 0 through 65 for every underlying primitive comparison operation and all three operand shapes.

Are these changes tested?

Yes. The compute subtree tests, focused race tests, 100 repeated regression runs, and linux/amd64 noasm compilation pass. The regression also verifies that bits outside the output range remain unchanged.

Are there any user-facing changes?

Pure-Go numeric and decimal comparisons no longer panic or produce incorrect output for non-byte-aligned result offsets. There is no API change.

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 16, 2026 12:59
@fallintoplace
fallintoplace force-pushed the fix/comparison-offset-bounds branch from 5c80111 to 632c218 Compare July 17, 2026 00:15

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The pure-Go fix here is correct and nicely tested — handling the leading prefix := offset % 8 sub-batch before the aligned batch loop is exactly right, and the temp-buffer handling is clean.

The concern is that this only fixes the pure-Go path. On the default amd64 build the comparison kernels dispatch to the generated SIMD assembly (scalar_comparison_avx2_amd64.s / scalar_comparison_sse4_amd64.s, generated from _lib/scalar_comparison.cc), which still carries the original output-offset bug:

  • num_batches = length / kBatchSize is computed from the original length before any prefix handling, and the tail loop iterates over the original length.
  • With a non-zero output offset (e.g. length=1, offset%8==7) the SIMD path can read past the input and/or corrupt bits beyond the intended output span.

Because the new tests call the pure-Go helpers directly, they pass green even though the default amd64 dispatch is still broken.

Requested changes before merge:

  1. Apply the same prefix/offset handling in _lib/scalar_comparison.cc and regenerate scalar_comparison_avx2_amd64.s / scalar_comparison_sse4_amd64.s.
  2. Add a kernel-level test that exercises the real dispatch (not the pure-Go helper directly) with a non-zero output offset, so the amd64 SIMD path is actually covered.

)

tmpOutSlice := tmpOutput[:]
if prefix := offset % 8; prefix != 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This prefix handling is correct for the pure-Go path — but the same fix needs to land in _lib/scalar_comparison.cc and the regenerated *_amd64.s, which is what the default amd64 build actually dispatches to. On amd64, num_batches/tail are still derived from the original length, so a non-zero output offset still over-reads / corrupts bits. See the main review comment for the full ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants