zeroshade commented on code in PR #1321:
URL: https://github.com/apache/arrow-go/pull/1321#discussion_r4085706671
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -363,6 +387,10 @@ func (d *DeltaByteArrayDecoder) Decode(out
[]parquet.ByteArray) (int, error) {
prefix := d.lastVal[:prefixLen:prefixLen]
if len(out[0]) == 0 {
+ // Decoded values must not escape through reusable
discard storage.
+ if len(prefix) > 0 && len(d.discardScratch) > 0 &&
&prefix[0] == &d.discardScratch[0] {
+ prefix = slices.Clone(prefix)
+ }
Review Comment:
This guard is doing more work than the comment lets on, and it's new — on
`main` this path is safe for free because `Discard` never reuses a buffer.
I verified it's not decorative: deleting these four lines makes
`TestDeltaByteArrayDecoderDiscardKeepsDecodedPrefixes` fail, and a randomized
interleaved `Decode`/`Discard` stress test I wrote reports 44 corrupted values
across 400 seeds. With the guard, both pass.
Suggest expanding the comment to say *why* — something like "after
`Discard`, `lastVal` aliases the reusable scratch buffer; a later `Discard`
would overwrite a value the caller still holds" — so the next person doesn't
read it as a defensive nicety and delete it.
Optional alternative: hoisting the check to the top of `Decode` (clone
`d.lastVal` once if it aliases scratch) would make this structural — "`lastVal`
never aliases scratch once `Decode` starts" — rather than a per-value check
inside the loop. Costs one clone in cases that don't need it, so the current
lazy form is strictly cheaper. Your call; I'd take either.
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -173,6 +175,25 @@ func (DeltaByteArrayDecoder) Type() parquet.Type {
func (d *DeltaByteArrayDecoder) Allocator() memory.Allocator { return d.mem }
+func (d *DeltaByteArrayDecoder) setDiscardLastValue(prefix, suffix
parquet.ByteArray) {
+ valueLen := len(prefix) + len(suffix)
+ if valueLen == 0 {
+ if d.discardScratch == nil {
+ d.discardScratch = make([]byte, 0, 1)
Review Comment:
Nit: the `, 1` isn't needed — `make([]byte, 0)` already returns a non-nil
slice, which is all this branch needs to keep `lastVal != nil` working as the
"first value of the page" sentinel. Harmless either way; if the intent was to
signal "must be a real allocation, not nil", a short comment would carry that
better than the capacity.
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -162,8 +163,9 @@ func (enc *DeltaByteArrayEncoder) FlushValues() (Buffer,
error) {
type DeltaByteArrayDecoder struct {
*DeltaLengthByteArrayDecoder
- prefixLengths []int32
- lastVal parquet.ByteArray
+ prefixLengths []int32
+ lastVal parquet.ByteArray
+ discardScratch []byte
Review Comment:
Worth documenting the invariant that makes the `slices.Clone` guard in
`Decode` sufficient:
> Every `lastVal` produced by `Discard` begins at `&discardScratch[0]` —
`setDiscardLastValue` assigns the whole buffer, and the empty-suffix branch
narrows with `lastVal[:prefixLen:prefixLen]`, which preserves the base pointer.
That's what lets the guard get away with comparing only `&prefix[0] ==
&d.discardScratch[0]` instead of a range check. It holds today, and I couldn't
construct a counterexample. But it's load-bearing and stated nowhere, so a
future change to `Discard` that leaves `lastVal` at a non-zero offset into the
scratch would defeat the guard *silently* — corrupted values, no panic, no
failing assertion unless someone happens to hit the right interleaving.
A couple of sentences here (or on `setDiscardLastValue`) would make that
much harder to break by accident.
##########
parquet/internal/encoding/delta_byte_array.go:
##########
@@ -246,23 +264,29 @@ func (d *DeltaByteArrayDecoder) Discard(n int) (int,
error) {
}
prefix := d.lastVal[:prefixLen:prefixLen]
- if _, err := d.DeltaLengthByteArrayDecoder.Decode(tmp); err !=
nil {
- return n - remaining, err
- }
-
- if len(tmp[0]) == 0 {
+ suffix := d.decodeDiscardSuffix()
+ if len(suffix) == 0 {
d.lastVal = prefix
} else {
- d.lastVal = make([]byte, int(prefixLen)+len(tmp[0]))
- copy(d.lastVal, prefix)
- copy(d.lastVal[prefixLen:], tmp[0])
+ d.setDiscardLastValue(prefix, suffix)
}
remaining--
}
return n, nil
}
+// decodeDiscardSuffix reads one suffix after Discard has bounded its count by
+// nvals. SetData has already validated the suffix lengths against the payload.
+func (d *DeltaByteArrayDecoder) decodeDiscardSuffix() parquet.ByteArray {
+ length := d.lengths[0]
+ suffix := d.data[:length:length]
+ d.data = d.data[length:]
+ d.nvals--
+ d.lengths = d.lengths[1:]
+ return suffix
+}
Review Comment:
Consider putting this on `DeltaLengthByteArrayDecoder` instead. Every field
it touches (`lengths`, `data`, `nvals`) is owned by that type, and the body
duplicates its `Decode` loop for n=1 — so the two can drift independently. A
`decodeOne()` there with the same "caller must ensure `nvals > 0`" precondition
keeps them in sync by construction.
Not a correctness issue: `nvals` and `len(lengths)` move in lockstep
(`SetData` establishes `nvals == len(lengths)`, `Discard` bounds itself by
`min(n, d.nvals)`, and both decrement by one per iteration), so `d.lengths[0]`
can't go out of range here. The unchecked index just reads scarier than it is,
which is another argument for moving it next to the invariant it relies on.
--
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]
