zeroshade commented on code in PR #1192:
URL: https://github.com/apache/arrow-go/pull/1192#discussion_r3799295948
##########
parquet/file/column_writer.go:
##########
@@ -397,9 +397,20 @@ func (w *columnWriter) buildDataPageV2(defLevelsRLESize,
repLevelsRLESize, uncom
}
// concatenate uncompressed levels and the possibly compressed values
- var combined bytes.Buffer
- combined.Grow(int(int64(defLevelsRLESize) + int64(repLevelsRLESize) +
int64(len(data))))
- w.concatBuffers(defLevelsRLESize, repLevelsRLESize, data, &combined)
+ bufferedPage := w.hasDict && !w.fallbackToNonDict
+ combinedSize := int(int64(defLevelsRLESize) + int64(repLevelsRLESize) +
int64(len(data)))
+ var combined []byte
+ if bufferedPage {
+ var owned bytes.Buffer
+ owned.Grow(combinedSize)
+ w.concatBuffers(defLevelsRLESize, repLevelsRLESize, data,
&owned)
+ combined = owned.Bytes()
+ } else {
+ w.uncompressedData.Reset()
+ w.uncompressedData.Grow(combinedSize)
Review Comment:
`bytes.Buffer.Reset()` retains capacity, so this introduces one page-sized
high-water allocation for every live DataPageV2 column writer. That is not
visible in the per-operation allocation benchmark.
In a buffered row group with 200 live columns and `DataPageSize=1 MiB`,
post-GC heap usage increased by approximately 200 MiB versus the merge base—one
retained MiB per column. This brings V2 in line with the existing V1 behavior,
but it is still a material new peak-memory trade-off for wide schemas.
Could we avoid retaining scratch independently in every column writer, such
as through pooled/shared eager-page scratch? At minimum, please clear
`uncompressedData` in `columnWriter.Close()` and document the retained-memory
trade-off alongside the allocation reduction.
##########
parquet/file/file_writer_test.go:
##########
@@ -312,7 +312,11 @@ func TestBufferedMultiPageDisabledDictionary(t *testing.T)
{
)
var (
sink = encoding.NewBufferWriter(0, memory.DefaultAllocator)
- props =
parquet.NewWriterProperties(parquet.WithDictionaryDefault(false),
parquet.WithDataPageSize(pageSize))
+ props = parquet.NewWriterProperties(
+ parquet.WithDictionaryDefault(false),
+ parquet.WithDataPageVersion(parquet.DataPageV2),
Review Comment:
This converts the existing buffered, multi-page, dictionary-disabled
regression test from the default DataPageV1 path to DataPageV2, removing its
previous V1 coverage.
Please make the test table-driven over both `DataPageV1` and `DataPageV2`
rather than replacing one with the other. For the V2 case, coverage with
non-empty definition/repetition levels or compression would also better
exercise concatenation into the reused buffer across page boundaries.
--
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]
