zeroshade commented on issue #1255:
URL: https://github.com/apache/arrow-go/issues/1255#issuecomment-5457495259
Root-caused. The file on disk is fine — this is purely a read-side bug, and
the encoder is not involved.
## Bisect
Reading the *same* written file through the low-level `ReadBatch` API
returns correct data for all 5000 rows, multi-page and nulls included. Only the
record-reader / `DecodeSpaced` path is wrong. That rules out the writer and the
on-disk bytes.
## Root cause
`spacedExpand` moves values into their spaced positions with `copy`, and
deliberately does not clean up afterwards:
```go
// because we technically don't care what is in the null slots we don't
actually have to clean
// up after ourselves ... Any data that happens to be left in the null
// slots is fine since it shouldn't matter and saves us work.
n := copy(buffer[run.Pos:], buffer[idxDecode:int64(idxDecode)+run.Length])
```
For scalar column types that reasoning holds — the buffer holds values, and
a stale value in a null slot is harmless.
For `ByteArray` / `FixedLenByteArray` the buffer holds **slice headers**, so
`copy` leaves *duplicate headers* behind: after expansion, a null slot and a
valid slot can reference the **same backing array**.
That is still harmless as long as the decoder *replaces* the header.
`PlainFixedLenByteArrayDecoder.Decode` does exactly that:
```go
out[idx] = pflba.data[:pflba.typeLen] // replaces the header
```
But `ByteStreamSplitFixedLenByteArrayDecoder` writes **through** the
caller's existing slice:
```go
out[idx] = out[idx][:dec.typeLen] // reuses the caller's storage
...
out[element][stream] = data[encLoc] // writes through it
```
So when the record reader reuses `flbaRecordReader.valueBuf` for the next
page, two output slots share one backing array, and the second value written
clobbers the first.
## Minimal demonstration
Two successive `DecodeSpaced` calls on one reused 5-slot buffer, 2 nulls,
valid bits `0b00010101`:
```
BSS: alias after page1: slots [1 2] share backing 0x...
page2 slot 2: got "lmno" want "hijk" <-- value from slot 3
appears in slot 2
PLAIN: alias after page1: slots [1 2] share backing 0x...
page2: OK <-- same aliasing, but
harmless
```
Slots 1 and 2 alias after page 1. On page 2, `Decode` writes values 1 and 2
through those two headers into the same 4 bytes; the later write wins, and
`spacedExpand` then shifts that slot's header into place — producing exactly
the observed off-by-one. It also explains every part of the isolation: BSS-only
(PLAIN replaces headers), nulls required (no nulls means no duplicate headers),
and multiple pages required (the first page creates the aliases, the second
decodes into them).
## Fix
The general fix — making `spacedExpand` swap rather than copy, so the buffer
stays a permutation with no duplicates — works, but is far too expensive.
Replacing `memmove` with element-wise swaps costs +1250% to +4280% on
sparse-null runs:
```
SpacedExpandInt64/n65536/nullEvery0 4.457µ -> 81.265µ +1723%
SpacedExpandFLBA/n65536/nullEvery0 3.969µ -> 173.959µ +4283%
```
The targeted fix is one line in the only decoder that writes through caller
storage, and leaves `spacedExpand` (and every other type) untouched:
```go
func (dec *ByteStreamSplitFixedLenByteArrayDecoder) DecodeSpaced(out
[]parquet.FixedLenByteArray, nullCount int, validBits []byte, validBitsOffset
int64) (int, error) {
toRead := len(out) - nullCount
// spacedExpand scatters slice headers by copying, leaving duplicate
headers in the
// null slots. This decoder writes *through* the caller's slices rather
than
// replacing them, so reusing a buffer that still holds those
duplicates would make
// two output slots share one backing array and clobber each other.
clear(out[:toRead])
valuesRead, err := dec.Decode(out[:toRead])
...
```
Verified: the reproducer above passes, and `parquet/internal/encoding`,
`parquet/file`, and `parquet/pqarrow` all pass with `PARQUET_TEST_DATA`
supplied.
### Interaction with #1172
`clear` forces every spaced decode down the cold path, so the cost depends
on whether #1172 has landed. Measured on 4096 slots / 3511 values / width 16:
| base | allocs/op for `DecodeSpaced` |
| --- | --- |
| `main` today (buggy, reuses headers) | 0 |
| `main` + this fix | 3511 |
| #1172 + this fix | **1** |
#1172's contiguous cold-output allocation collapses the cost of the fix to a
single allocation, so it is worth landing #1172 first and this fix on top of it.
I have not opened a PR for the fix yet — flagging the analysis first in case
there is a preferred approach (for example fixing this at the
`flbaRecordReader` level instead, or making the "decoders must not write
through caller storage after `spacedExpand`" contract explicit somewhere).
--
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]
