zeroshade commented on code in PR #1031:
URL: https://github.com/apache/arrow-go/pull/1031#discussion_r3660789913
##########
parquet/internal/encoding/encoding_test.go:
##########
@@ -1089,6 +1089,14 @@ func TestBooleanPlainDecoderAfterFlushing(t *testing.T) {
assert.Equal(t, decSlice[0], false)
}
+func TestBooleanPlainDecoderRejectsTruncatedData(t *testing.T) {
+ descr := schema.NewColumn(schema.NewBooleanNode("bool",
parquet.Repetitions.Required, -1), 0, 0)
+ dec := encoding.NewDecoder(parquet.Types.Boolean,
parquet.Encodings.Plain, descr, memory.DefaultAllocator)
+
+ assert.ErrorContains(t, dec.SetData(9, []byte{0xff}), "need 2 for 9
values")
Review Comment:
This test encodes the same incorrect invariant, which is why it passes in
isolation — it constructs the decoder directly, so `nvals` really does equal
the physical count here. The nullable path through `columnChunkReader` is where
the assumption breaks, and no test in this package covers it.
Separately, asserting on the substring `"need 2 for 9 values"` couples the
test to the exact wording of the error message. `require.ErrorIs` against a
sentinel, or a looser match, would be less brittle.
##########
parquet/internal/encoding/boolean_decoder.go:
##########
@@ -43,6 +43,13 @@ func (PlainBooleanDecoder) Type() parquet.Type {
}
func (dec *PlainBooleanDecoder) SetData(nvals int, data []byte) error {
+ if nvals < 0 {
+ return fmt.Errorf("parquet: invalid number of boolean values:
%d", nvals)
+ }
+ required := bitutil.BytesForBits(int64(nvals))
+ if int64(len(data)) < required {
+ return fmt.Errorf("parquet: boolean data is too short: got %d
bytes, need %d for %d values", len(data), required, nvals)
+ }
Review Comment:
This is the check that breaks valid files. `nvals` here is the page's level
count (`c.numBuffered`, from the header's `num_values`, nulls included) — not
the number of physical booleans in the payload. For a nullable column the PLAIN
payload holds only non-null bits, so `BytesForBits(nvals)` legitimately exceeds
`len(data)`.
Concretely, 100 rows with 4 non-null gives a 1-byte payload while this
demands 13 bytes.
The `nvals < 0` guard just above is fine — it's this length comparison that
needs to move into `Decode`/`Discard`/`DecodeToBitmap`, where the real count of
values being read is known and can be bounded against `len(dec.data)`.
--
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]
