zeroshade commented on code in PR #1766:
URL: https://github.com/apache/iceberg-go/pull/1766#discussion_r3831718704
##########
table/partitioned_fanout_writer.go:
##########
@@ -520,6 +520,22 @@ func partitionBatchByKey(ctx context.Context)
partitionBatchFn {
mem := compute.GetAllocator(ctx)
return func(record arrow.RecordBatch, rowIndices []int64)
(arrow.RecordBatch, error) {
+ if len(rowIndices) == 0 && record.NumRows() == 0 {
+ record.Retain()
+
+ return record, nil
+ }
+
+ if start, end, ok := contiguousRowRange(rowIndices,
record.NumRows()); ok {
+ if start == 0 && end == record.NumRows() {
+ record.Retain()
+
+ return record, nil
+ }
+
+ return record.NewSlice(start, end), nil
Review Comment:
`RecordBatch.NewSlice` retains the source arrays' complete backing buffers,
not just `[start:end]`. `RollingDataWriter.Add` then retains this record and
can queue 64 records asynchronously, so a slow partition containing only a tiny
contiguous range can pin up to 64 complete input batches. A checked-allocator
probe selecting one row from a 100,000-row string batch retained 34,095,168
bytes after releasing the source record; the previous `compute.Take` path
retained 384 bytes. The synchronous benchmark does not measure this queued
peak-memory behavior. Could we keep the full-record fast path but use `Take`
(or another bounded-copy policy) for disproportionately small partial ranges,
and add a queued-writer memory regression test?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
