zeroshade opened a new pull request, #1256: URL: https://github.com/apache/arrow-go/pull/1256
### Rationale for this change Fixes #1255. A `BYTE_STREAM_SPLIT` `FIXED_LEN_BYTE_ARRAY` column that contains nulls **and** spans more than one data page decodes values shifted by one position — silently, with no error returned. `spacedExpand` moves values into their spaced positions with `copy` and deliberately does not clean up the null slots: ```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 ``` That reasoning holds for the scalar column types, whose buffers hold values. But `ByteArray` / `FixedLenByteArray` buffers hold **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 for a decoder that *replaces* the header, which is what `PlainFixedLenByteArrayDecoder` does: ```go out[idx] = pflba.data[:pflba.typeLen] // replaces ``` `ByteStreamSplitFixedLenByteArrayDecoder` instead writes **through** the caller's existing slice: ```go out[idx] = out[idx][:dec.typeLen] // reuses caller storage ... out[element][stream] = data[encLoc] // writes through it ``` So once `flbaRecordReader` reused its value buffer for the next page, two output slots shared one backing array and clobbered each other. This explains the full shape of the bug: BYTE_STREAM_SPLIT only, nulls required (to create the duplicates), and two or more pages required (the first page creates the aliases, the second decodes into them). ### What changes are included in this PR? - Add `spacedExpandSwap`, which swaps instead of copying so the buffer remains a permutation of its original elements — no slot aliases another, and every slot keeps its reusable capacity. - Use it from `ByteStreamSplitFixedLenByteArrayDecoder.DecodeSpaced`. - Leave `spacedExpand` itself untouched, so every other column type and the non-spaced path are unaffected. - Add a decoder-level regression test (two pages through one reused buffer, widths 2/3/4/7/8/16) and a randomized differential test asserting `spacedExpandSwap` places values in exactly the same slots as `spacedExpand` while never leaving duplicates. - Add a `pqarrow` round-trip test over a multi-page, nullable BSS FLBA column — the integration-level case that was returning wrong data. The decoder-level regression test fails at every width without the fix. ### Why swap rather than the simpler alternatives? I measured two other approaches and rejected both: **Making `spacedExpand` itself swap** is correct but replaces `memmove` with element-wise swaps for *every* column type, which is far too expensive on sparse-null runs: ``` SpacedExpandInt64/n65536/nullEvery0 4.457µ -> 81.265µ +1723% SpacedExpandFLBA/n65536/nullEvery0 3.969µ -> 173.959µ +4283% ``` **`clear(out[:toRead])` in `DecodeSpaced`** is a one-liner, but discards all reusable headers. On 4096 slots / 3511 values / width 16 that is 3511 allocations per `DecodeSpaced` on `main` today (it would drop to 1 once #1172 lands, but the fix should not depend on that). Swapping keeps the reuse. Cost on the affected path only (`BenchmarkBSSFLBADecodeSpaced`, 8192 slots): ``` w4/nullEvery100 41.64µ -> 60.55µ +45% w4/nullEvery7 49.45µ -> 71.95µ +46% w4/nullEvery2 95.83µ -> 89.59µ -7% w16/nullEvery100 213.9µ -> 385.2µ +80% w16/nullEvery7 244.7µ -> 251.3µ ~ w16/nullEvery2 154.3µ -> 160.4µ ~ geomean +25% ``` Steady-state allocations stay at 1–2 per call. A ~25% geomean cost on a path that is currently returning **incorrect data** seemed clearly worth it, and nothing outside BSS FLBA spaced decoding is touched. ### Are these changes tested? Yes — new tests described above. `parquet/...` passes in full with `PARQUET_TEST_DATA` supplied, and `-race` is clean on `parquet/internal/encoding`. ### Are there any user-facing changes? Yes: BYTE_STREAM_SPLIT FIXED_LEN_BYTE_ARRAY columns with nulls spanning multiple data pages now decode correctly. Previously affected reads returned silently incorrect values. **This PR contains a "Critical Fix".** Reading an affected file produced wrong values with no error, which could have been persisted or acted on downstream without any indication of a problem. -- 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]
