zeroshade commented on code in PR #1182:
URL: https://github.com/apache/arrow-go/pull/1182#discussion_r3806529140
##########
parquet/file/record_reader.go:
##########
@@ -229,6 +228,9 @@ func (pr *primitiveRecordReader) ResetValues() {
}
func (pr *primitiveRecordReader) numBytesForValues(nitems int64) (num int64,
err error) {
+ if pr.Descriptor().PhysicalType() == parquet.Types.Boolean {
Review Comment:
**Blocking:** this silently changes the representation returned by the
exported `file.RecordReader.Values()` and `ReleaseValues()` APIs for Boolean
columns.
Previously, Boolean values used one byte per value, consistent with
`Boolean.ByteSize() == 1`. After this change they are LSB-first packed bits.
External code continues compiling but either reads garbage or indexes beyond
the shorter buffer. `file.NewRecordReader` is exported, so this is observable
outside pqarrow.
Please preserve the existing methods’ representation and expose the packed
path through a Boolean-specific interface such as `ReleaseValueBitmap()`, which
`pqarrow.transferBool` can detect and consume zero-copy. If this behavior break
is intentionally accepted instead, it at least needs explicit API documentation
and a release-note callout, but preserving compatibility would be preferable.
##########
parquet/pqarrow/boolean_bitmap_test.go:
##########
@@ -0,0 +1,90 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package pqarrow
+
+import (
+ "bytes"
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/arrow-go/v18/parquet"
+ "github.com/apache/arrow-go/v18/parquet/compress"
+ "github.com/apache/arrow-go/v18/parquet/file"
+
+ "github.com/stretchr/testify/require"
+)
+
+func TestBooleanBitmapReadAcrossPages(t *testing.T) {
Review Comment:
The cross-page test exercises the PLAIN bitmap decoder only.
`RleBooleanDecoder.DecodeToBitmap` is also new, but the existing RLE
file-reader test decodes through `[]bool` rather than through this pqarrow
bitmap-transfer path.
Please parameterize this regression test over PLAIN and RLE encoding so both
implementations are covered end to end across page boundaries.
##########
parquet/internal/encoding/boolean_decoder.go:
##########
@@ -205,6 +206,39 @@ func (dec *PlainBooleanDecoder) DecodeSpaced(out []bool,
nullCount int, validBit
return dec.Decode(out)
}
+func decodeSpacedToBitmap(dec BooleanBitmapDecoder, out []byte, outOffset
int64,
+ length, nullCount int, validBits []byte, validBitsOffset int64) (int,
error) {
+ if nullCount == 0 {
+ return dec.DecodeToBitmap(out, outOffset, length)
+ }
+
+ valuesToRead := length - nullCount
+ valuesRead, err := dec.DecodeToBitmap(out, outOffset, valuesToRead)
+ if err != nil {
+ return valuesRead, err
+ }
+ if valuesRead != valuesToRead {
+ return valuesRead, errors.New("parquet: boolean decoder: number
of values / definition levels read did not match")
+ }
+
+ // Expand the packed physical values backwards into their logical
positions.
+ // Copying backwards keeps unread compact values from being overwritten.
+ physicalIndex := valuesToRead - 1
+ for logicalIndex := length - 1; logicalIndex >= 0 && physicalIndex >=
0; logicalIndex-- {
+ if bitutil.BitIsSet(validBits,
int(validBitsOffset)+logicalIndex) {
+ value := bitutil.BitIsSet(out,
int(outOffset)+physicalIndex)
+ bitutil.SetBitTo(out, int(outOffset)+logicalIndex,
value)
Review Comment:
This per-bit backwards expansion regresses sparsely-null Boolean columns
despite the large dense/50%-null wins.
On an Apple M4 with `GOMAXPROCS=1`, the PR’s existing 1M-row, 10%-null
benchmark was consistently approximately 4.86 ms versus 4.64 ms on the merge
base—about 5% slower. An independent 1%-null run measured approximately 3.28 ms
versus 2.62 ms, about 25% slower. Allocated bytes still improve, but sparse
nulls are a common case and the PR description currently reports only dense and
50%-null results.
Please add 1% and 5% null-density cases and either avoid the regression or
explicitly document and justify the latency-versus-memory trade-off.
--
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]
