Skip to content

perf(arrow/array): skip full-range SliceEqual slices - #1249

Open
fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/arrow-slice-equal-full-range
Open

perf(arrow/array): skip full-range SliceEqual slices#1249
fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/arrow-slice-equal-full-range

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Skip temporary arrays when both ranges cover their full inputs.
  • Keep the existing NewSlice path for partial ranges.
  • This also helps ChunkedEqual when chunk boundaries line up.
  • Add full-range semantic coverage and benchmarks.

Benchmark

Local Apple M1 Pro. -benchtime=500ms -count=3.

Case main change
SliceEqual int64, 64 values ~700 ns, 352 B, 4 allocs ~516 ns, 0 B, 0 allocs
SliceEqual string, 64 values ~677 ns, 384 B, 4 allocs ~474 ns, 0 B, 0 allocs
ChunkedEqual, 1,024 chunks x 64 values ~756 us, 360 KB, 4,096 allocs ~559 us, 0 B, 0 allocs

Tests

  • go test ./arrow/array
  • go test -race ./arrow/array
  • go vet -composites=false ./arrow/array
  • go test ./... -run '^$'
  • GOOS=linux GOARCH=386 go test -c -o /dev/null ./arrow/array

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

Skipping slice construction for full-range inputs changes compatibility with arbitrary arrow.Array implementations.

The previous path normalized inputs through NewSlice/MakeFromData. The new fast path calls Equal directly, which panics for valid wrapper implementations it does not recognize. I reproduced this with a wrapper around *array.Int64; the same issue propagates through one-chunk ChunkedEqual. Details are inline.

This head currently has no CI results.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.

Comment thread arrow/array/compare.go Outdated

// SliceEqual reports whether slices left[lbeg:lend] and right[rbeg:rend] are equal.
func SliceEqual(left arrow.Array, lbeg, lend int64, right arrow.Array, rbeg, rend int64) bool {
if lbeg == 0 && lend == int64(left.Len()) && rbeg == 0 && rend == int64(right.Len()) {

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 full-range fast path calls Equal on the original interface values. Equal only handles Arrow-Go’s concrete array types or ExtensionArray, so another valid arrow.Array implementation now panics. Wrapping an *array.Int64 in struct{ arrow.Array } and calling full-range SliceEqual previously returned true; this head panics with arrow/array: unknown array type. Please retain the slice-normalization path for unsupported implementations or make Equal safely normalize generic arrays, and add the wrapper regression test.

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