zeroshade commented on PR #1256:
URL: https://github.com/apache/arrow-go/pull/1256#issuecomment-5511973049
@fallintoplace confirmed, and thank you — you were right, and the underlying
problem is broader than either of the two cases you named. Pushed 09060510.
I wrote your `RLE_DICTIONARY -> BYTE_STREAM_SPLIT` repro as a test first. It
fails on the previous commit exactly as you predicted, at every width:
```
--- FAIL: .../width=2/after_RLE_DICTIONARY
expected: parquet.FixedLenByteArray{0x64, 0x65}
actual : parquet.FixedLenByteArray{0x66, 0x67}
Messages: slot 0 decoded incorrectly
```
`dictConverter.Copy` does `o[idx] = dc.dict[val]`, and `Fill` copies one
header across an RLE run, so repeated indices leave several slots sharing one
dictionary-backed slice. And as you said, no nulls needed.
While writing it I checked `PLAIN -> BYTE_STREAM_SPLIT` again and found my
previous commit had only papered over it. `spacedExpandSwap` removes
*duplicate* headers, but PLAIN's `Decode` does `out[idx] =
pflba.data[:typeLen]`, so every slot points into the page buffer even with no
duplicates at all. BSS then overwrites that buffer wholesale:
```
--- FAIL: .../width=2/after_PLAIN
expected: []byte{0x0, 0x1, 0x2, 0x3, 0x4, 0x5}
actual : []byte{0x64, 0x65, 0x66, 0x67, 0x68, 0x69}
Messages: decoding wrote through into the PLAIN page buffer
```
So the root cause isn't aliasing specifically — it's that a decoder which
writes *through* caller headers was claiming them on capacity alone, while the
value buffer is shared across pages and encodings. Capacity can't distinguish
our storage from a dictionary entry or a page buffer.
Reuse is now gated on provenance: a header is only written through if it
falls inside the block this decoder allocated; anything else is re-pointed at a
fresh block.
### This narrows your GH-1172 contract, so please push back if you disagree
`TestByteStreamSplitFixedLenByteArrayDecoderReusesProvidedOutput` asserted
that a caller-provided buffer with capacity gets filled in place. That is
precisely the unsafe behavior, and I could not find a way to keep it — the
record reader hands us the same `valueBuf` whether the headers are ours or were
left by the previous page's decoder. I replaced it with two tests: one pinning
that we do *not* write through caller buffers, one pinning that we still reuse
our own storage.
Allocation is unchanged where it matters. Preparing only the decoded prefix
regressed `DecodeSpaced` to 1 alloc/op, because the expansion permutes headers
across the whole window and unowned ones migrate into the prefix. Preparing the
whole window once, null slots included, restores it:
| | allocs/op | correct? |
| --- | --- | --- |
| before this commit | 0 | **no** |
| prefix-only preparation | 1 | yes |
| this commit | **0** | yes |
(4096 slots / 3009 values / width 16, steady state.) The dense `Decode` path
is untouched — successive windows still allocate one block each, as before, and
earlier windows keep their values.
`parquet/...` and `arrow/...` pass in full, `-race` is clean on `encoding`,
`file` and `pqarrow`, and `golangci-lint` reports 0 issues. Both new tests fail
without the fix.
One thing I'd like your read on: `owns` compares `unsafe.SliceData` pointers
against the block bounds. It's the honest expression of "did I allocate this",
and `go vet` is clean, but if you'd rather not have pointer-provenance in this
path I'm open to alternatives — the only other one I found that is clearly
correct costs an allocation per page.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
