peterxcli commented on PR #5046:
URL:
https://github.com/apache/datafusion-comet/pull/5046#issuecomment-5207910547
@andygrove pretty thanks for another round of review!
> `writeColUnsafe` is only in-bounds when the writer came from `create(root,
capacity)`, and nothing connects those two. `RowArrowReader`,
`CometArrowConverters`, and `ConstantColumnVectors.materialize` all build
writers with the plain `create(root)` and its 3970 default, so pointing any of
them at the new methods later would compile fine and write out of bounds.
> Have you considered folding the capacity guarantee into `writeCol`
instead? The base `writeCol` already calls
`valueVector.setInitialCapacity(inputNumElements)`, and as far as I can tell
that call does nothing at that point, since `setInitialCapacity` only affects
the next `allocateNew()` and we are already past it. If a
`FixedWidthArrowFieldWriter` override replaced it with something like:
> ```scala
> while (valueVector.getValueCapacity < inputNumElements)
valueVector.reAlloc()
> ```
> then the non-resizing setters would always be in-bounds by construction.
That would let `writeColUnsafe` and `writeColNoNullUnsafe` go away entirely,
and `ConstantColumnVectors.materialize` would pick up the same win, which
matters because it runs per batch for partition and `_metadata` constants.
Addressed, this is brilliant. great cleanup.
> Could the capacity check in `writeColUnsafe` be `require` rather than
`assert`? `NativeBase.setArrowProperties()` sets
`arrow.enable_unsafe_memory_access=true` whenever debug mode is off, which is
the default, and that disables Arrow's own `ArrowBuf` bounds checks. So that
line is the only guard against an out-of-bounds native write if a writer ever
reaches there under-allocated. It runs once per column per batch and
`getValueCapacity` is just a cached field read, so making it unconditional
should not show up in the numbers.
make sense, changed assert to `while (valueVector.getValueCapacity <
inputNumElements) { valueVector.reAlloc() }`
> Since `create(root, capacity)` exists now, is there a reason to leave
`RowArrowReader` on the plain `create(root)`? It knows `maxRecordsPerBatch`, so
today it allocates 3970 and reallocs to 8192 on every batch. That path is the
fallback for any child that does not support columnar output, so it may well be
hit more often than the columnar one.
you're right, remaining usage are only existed in test code. I've completely
removed the plain `create(root)` and change its caller code.
> Also, the class doc on `SparkColumnarArrowReader` still says the rows go
in "via `ArrowWriter.writeCol`". Worth updating to match.
class doc on `SparkColumnarArrowReader` is updated.
> benchmark: the A/B numbers are exactly what I wanted ... . ... there is
nothing for Spark to Arrow conversion.
Added as `CometArrowWriterBenchmark.scala`.
> `nullableArrow.getValueCapacity should be >= numRows` would pass on the
old resizing path too, since `reAlloc` grows the vector past `numRows` either
way. The assertion that actually pins the new behavior is `allocatedBytes
shouldBe allocator.getAllocatedMemory`, and its intent is not obvious from
reading it. A short comment saying that a realloc would make cumulative
allocations exceed live allocated memory would help whoever touches this next.
Added into existing "pre-sized Arrow writer avoids fixed-width
reallocations" test case in `CometArrowStreamSuite.scala`.
> It might also be worth extending this beyond `IntVector`. `BooleanWriter`
and `DecimalWriter` have the only non-trivial `setValueUnsafe` bodies,
`BitVector` writes bits into the value buffer rather than fixed-width slots,
and
Added into existing "pre-sized Arrow writer avoids fixed-width
reallocations" test case in `CometArrowStreamSuite.scala`.
> `DecimalWriter`'s `changePrecision`-returns-false branch calling
`setNullUnsafe` is not exercised anywhere I can find. Comparing all `numRows`
values against the input instead of three spot checks would also catch an
off-by-one in the new loops.
Added in `CometArrowStreamSuite.scala` as a new "fixed-width decimal write
clears validity on precision overflow" test case.
> Lastly, given the point about `arrow.enable_unsafe_memory_access` above, a
test that a mis-sized writer fails loudly would be worth having. Build one with
`create(root)`, then write more than 3970 rows.
Added in `CometArrowStreamSuite.scala` as a new "fixed-width column write
grows an undersized vector" test case.
--
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]
