zeroshade commented on code in PR #880:
URL: https://github.com/apache/arrow-go/pull/880#discussion_r3560514384
##########
parquet/file/page_reader.go:
##########
@@ -91,6 +99,43 @@ func (p *page) Data() []byte { return
p.buf.Bytes() }
func (p *page) NumValues() int32 { return p.nvals }
func (p *page) Encoding() format.Encoding { return p.encoding }
+// dataPageBase holds the members shared by DataPageV1/V2 but not dictionary
+// pages: the rep/def level buffer accessor and, in streaming mode, the
+// incremental value source.
+type dataPageBase struct {
+ page
+
+ // valueBuf is only set for streaming mode to read values incrementally.
+ // In such mode, buf only holds the rep+def level region.
+ valueBuf streaming.ValueBuffer
+ // clip bounds a read's batch to keep one Decode's aliased values near
the buffer
+ // size; 0 for non-streaming pages.
+ clip int64
+}
+
+func (d *dataPageBase) valueBuffer() streaming.ValueBuffer { return d.valueBuf
}
+func (d *dataPageBase) levelBuffer() []byte { return
d.buf.Bytes() }
+
+func (d *dataPageBase) valueClip() int64 { return d.clip }
+
+func (d *dataPageBase) setStreamingValues(mem memory.Allocator,
lenUncompressed int, levelBuf []byte, valReader, rawSrc io.Reader, closer
io.Closer) {
+ d.buf = memory.NewBufferBytes(levelBuf)
Review Comment:
**Allocator accounting: the streaming rep/def level buffer bypasses the
configured `memory.Allocator`.**
`setStreamingValues` wraps `levelBuf` with `memory.NewBufferBytes(...)`, but
`levelBuf` is `make`-allocated in both streaming paths (V2: `make([]byte,
levelsBytelen)`; V1: via `readLevelData`). Unlike the non-streaming data pages
— which wrap `p.dataPageBuffer.Bytes()` from an allocator-backed
`ResizableBuffer` (`memory.NewResizableBuffer(p.mem)`) — this level region
isn't tracked by the configured allocator, and `DataPage.Release()` can't
return it. Checked/custom allocators won't see it.
The streamed *value* region already flows through `p.mem` via
`streaming.NewStreamBuffer(mem, ...)`, so only the (usually small) level region
is unaccounted — real impact is limited to level-heavy deeply-repeated/nested
pages. It's still inconsistent with the non-streaming paths and slightly at
odds with this PR's memory-management goal.
Suggestion: allocate `levelBuf` through `p.mem` and store an
allocator-backed `*memory.Buffer` on the page so `DataPage.Release()` frees it.
_(Surfaced by an automated review pass — non-blocking; feel free to defer if
you consider the level region negligible.)_
--
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]
