You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "winklemad (via GitHub)" <gi...@apache.org> on 2026/08/27 03:55:20 UTC
[PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
winklemad opened a new pull request, #1219:
URL: https://github.com/apache/arrow-go/pull/1219
### Rationale for this change
`array.Concatenate` silently corrupts values when one of the inputs is a `RunEndEncoded` array that was sliced in the **middle of a run**.
`updateRuns` normalizes each input's run ends by subtracting its logical offset, but never clamps the final run end to the array's logical length. A slice keeps the original physical end of the run it cuts through, so after normalization the last run end overshoots the slice length. That overshoot then shifts every following array's run ends, and the result still passes `ValidateFull`, so nothing flags the corruption.
Reproduction (values are wrong, no error):
```go
b := array.NewRunEndEncodedBuilder(mem, arrow.PrimitiveTypes.Int32, arrow.PrimitiveTypes.Int64)
vb := b.ValueBuilder().(*array.Int64Builder)
// run ends [3,5,8]: 100x3, 200x2, 300x3
b.Append(3); vb.Append(100); b.Append(2); vb.Append(200); b.Append(3); vb.Append(300)
full := b.NewArray()
sliced := array.NewSlice(full, 1, 4) // logical [100,100,200], ends mid-run of the 200s
b.Append(2); vb.Append(700)
tail := b.NewArray()
result, _ := array.Concatenate([]arrow.Array{sliced, tail}, mem)
// want [100,100,200,700,700]
// got [100,100,200,200,700] <- index 3 corrupted
```
`sliced` on its own decodes correctly (`[100,100,200]`); only the concatenated result is wrong, so the defect is entirely in the run-end merge.
### What changes are included in this PR?
Clamp each input array's final run end to the running logical length in `updateRuns` (`arrow/array/concat.go`). This is the single place run ends are merged for `RunEndEncoded` concatenation (the generic function covers int16/int32/int64 run-end types). No change to any array that ends on a run boundary — only a slice that cuts through a run is affected, and it now stays within its logical length.
### Are these changes tested?
Yes — added `TestConcatRunEndEncodedMidRunSlice` in `arrow/array/concat_test.go`, which reproduces the corruption (it fails without the fix) and uses a checked allocator to confirm no leaks. The existing `TestConcatRunEndEncoded` / `TestConcatAlmostOverflowRunEndEncoding` and the full `arrow/array` package tests still pass; `gofmt` and `go vet` are clean.
### Are there any user-facing changes?
Yes — `array.Concatenate` now returns correct values when an input is a mid-run slice of a `RunEndEncoded` array, instead of silently wrong ones. No API change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1219:
URL: https://github.com/apache/arrow-go/pull/1219#discussion_r3896881529
##########
arrow/array/concat.go:
##########
@@ -866,21 +866,32 @@ func updateRuns[T int16 | int32 | int64](inputData []arrow.ArrayData, inputBuffe
continue
}
src := arrow.GetData[T](buf.Bytes())
+ offset := inputData[i].Offset()
+
+ // A slice can end in the middle of a run, leaving this input's final physical
+ // run end past its logical length. Clamp the normalized final run end to the
+ // input's logical length before both the overflow check and the output write:
+ // otherwise a valid near-limit slice trips a false overflow, and the written
+ // run end overshoots (shifting every following array's run ends).
+ finalEnd := int(src[len(src)-1]) - offset
Review Comment:
Converting the final `int64` run end to `int` overflows on 32-bit targets. A valid run-end encoded array with final run end `math.MaxInt64` and logical length 1 becomes `-1` during concatenation, producing invalid output. Please keep this calculation in type `T`, clamp against `T(inputData[i].Len())`, and add a 32-bit `int64` regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
Posted by "winklemad (via GitHub)" <gi...@apache.org>.
winklemad commented on PR #1219:
URL: https://github.com/apache/arrow-go/pull/1219#issuecomment-5491165504
Fixed in fe3b0ab. The final run end and the per-run normalization in the write loop now stay in the run-end type `T` and clamp against `T(inputData[i].Len())`, so nothing goes through `int` — the `MaxInt64` / logical-length-1 case no longer wraps to a negative value on 32-bit targets. Added `TestConcatRunEndEncodedInt64FinalRunEndClamp` for that scenario (it passes on a 64-bit host where `int` is 64-bit, and guards the 32-bit path). REE/concat suite + `go vet` are green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1219:
URL: https://github.com/apache/arrow-go/pull/1219#discussion_r3883854390
##########
arrow/array/concat.go:
##########
@@ -861,37 +861,44 @@ func updateRuns[T int16 | int32 | int64](inputData []arrow.ArrayData, inputBuffe
// can fold the end and beginning of each array we're concatenating
// into a single run
pos := 0
+ logicalLen := T(0)
for i, buf := range inputBuffers {
if buf.Len() == 0 {
continue
}
src := arrow.GetData[T](buf.Bytes())
+ logicalLen += T(inputData[i].Len())
if pos == 0 {
pos += copy(output, src)
// normalize the first run ends by subtracting the offset
for j := 0; j < pos; j++ {
output[j] -= T(inputData[i].Offset())
}
+ } else {
+ lastEnd := output[pos-1]
+ // we can check the last runEnd in the src and add it to the
+ // last value that we're adjusting them all by to see if we
+ // are going to overflow
+ if uint64(lastEnd)+uint64(int(src[len(src)-1])-inputData[i].Offset()) > uint64(maxOf[T]()) {
Review Comment:
The overflow check still uses the slice’s physical final run end before the new logical-length clamp. This rejects valid near-limit inputs. A valid `int16` REE prefix of length 32,760 followed by a one-element slice of a physical 32,767-element run should produce length 32,761, but currently returns `invalid: overflow in run-length-encoded run ends concat`. Please clamp the normalized final source end to the sliced input’s logical length before both the overflow check and output write, and add this boundary regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1219:
URL: https://github.com/apache/arrow-go/pull/1219
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): clamp run ends when concatenating a sliced RunEndEncoded array [arrow-go]
Posted by "winklemad (via GitHub)" <gi...@apache.org>.
winklemad commented on PR #1219:
URL: https://github.com/apache/arrow-go/pull/1219#issuecomment-5458266919
Thanks @zeroshade — you're exactly right, the overflow check was still measuring the unclamped physical run end. Fixed in 300dbe3: I compute the clamped final run end once up front (`min(normalized physical end, input.Len())`) and use it for **both** the overflow check and the written final run end (the raw write could otherwise overshoot/wrap before the post-clamp could catch it).
Your repro now passes — an `int16` prefix of 32,760 followed by a one-element slice of a physical 32,767-length run returns length 32,761 instead of the false overflow. Added `TestConcatRunEndEncodedNearTypeLimitSlice` for that boundary; the existing RunEndEncoded tests and the full `arrow/array` suite stay green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org