andygrove commented on code in PR #6281:
URL: https://github.com/apache/datafusion-comet/pull/6281#discussion_r4116821100
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala:
##########
@@ -485,9 +485,28 @@ private[arrow] class StringWriter(val valueVector:
VarCharVector) extends ArrowF
override def setValue(input: SpecializedGetters, ordinal: Int): Unit = {
val utf8 = input.getUTF8String(ordinal)
- val utf8ByteBuffer = utf8.getByteBuffer
- // todo: for off-heap UTF8String, how to pass in to arrow without copy?
- valueVector.setSafe(count, utf8ByteBuffer, utf8ByteBuffer.position(),
utf8.numBytes())
+ if (utf8.getBaseObject == null) {
+ val length = utf8.numBytes()
+ // Null entries may not have offsets yet. Match Arrow's append position
before reserving.
+ val start =
+ if (valueVector.getLastSet < 0) 0L
+ else valueVector.getStartOffset(valueVector.getLastSet + 1).toLong
+ require(
+ length >= 0 && start >= 0 && start + length <= Int.MaxValue,
+ "String column exceeds the 32-bit Arrow offset range")
Review Comment:
This pre-check re-derives Arrow's append position to guard the 32-bit offset
range, but `setValueLengthSafe` already enforces that bound. `handleSafe` asks
`reallocDataBuffer` for `start + length` bytes, and anything past
`Int.MaxValue` throws `OversizedAllocationException` before it allocates or
changes any state. I tried both versions against Arrow 18.3.0, in this suite's
overflow state and with two real 1.1 GiB strings in one vector. Without the
pre-check, the off-heap path fails with the same `OversizedAllocationException`
as the heap path and the old writer. With it, only off-heap batches fail with
`IllegalArgumentException`. So the same oversized batch reports a different
error depending on where its strings live. The `length >= 0` half is worth
keeping, because Arrow never checks it and a negative length silently writes an
end offset below the start. Could this become just `require(length >= 0, ...)`,
with the overflow test asserting `OversizedAllocationException` for all t
hree backings? That would keep heap and off-heap on the same failure, and the
writer wouldn't need to mirror `handleSafe`'s internals.
--
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]
