andygrove commented on code in PR #6730:
URL: https://github.com/apache/datafusion-comet/pull/6730#discussion_r4211294610
##########
spark/src/test/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowWriterSuite.scala:
##########
@@ -640,4 +656,146 @@ class CometArrowWriterSuite extends AnyFunSuite with
Matchers {
}
}
}
+
+ // Nested shapes whose unsafe forms take paths of their own: elements
converted one at a time,
+ // collections inside collections, and a struct wider than one word of null
bits.
+ private val moreNestedTypes: Seq[DataType] = Seq(
Review Comment:
Done in 61ec88b771. `moreNestedTypes` now starts with
`primitiveTypes.map(ArrayType(_))`, which also replaces the boolean,
decimal(38,10) and binary arrays that were listed by hand. I logged which
vectors reach the block copy while the suite runs. Before, only `IntVector` and
`BigIntVector` did. Now all twelve in `unsafeValueWidth` do, including
`TinyIntVector` and `SmallIntVector`, and `TimeNanoVector` on 4.1.
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala:
##########
@@ -1384,16 +1627,59 @@ private[arrow] class TimeNanoWriter(val valueVector:
TimeNanoVector)
private[arrow] class ArrayWriter(val valueVector: ListVector, val
elementWriter: ArrowFieldWriter)
extends ArrowFieldWriter {
+ // Pointed at each unsafe array in turn, rather than allocating a view per
value.
+ private val elements = new UnsafeArrayData
+
override def setNull(): Unit = {}
override def setValue(input: SpecializedGetters, ordinal: Int): Unit = {
- val array = input.getArray(ordinal)
+ input.getArray(ordinal) match {
+ case unsafe: UnsafeArrayData =>
+ writeElements(unsafe)
+ case array =>
+ val numElements = array.numElements()
+ valueVector.startNewValue(count)
+ var i = 0
+ while (i < numElements) {
+ elementWriter.write(array, i)
+ i += 1
+ }
+ valueVector.endValue(count, numElements)
+ }
+ }
+
+ override private[arrow] def writeUnsafeRowField(row: UnsafeRow, ordinal:
Int): Unit = {
Review Comment:
I tried this, and it costs more than I expected. As a private method in each
writer, `writeUnsafe` is bound statically and inlined. In a shared
`writeArrayElements` and `writeUnsafeRowField` it becomes one virtual call site
for all three writers, which goes megamorphic once a JVM has converted arrays,
maps and structs. I warmed every case together and ran four case orders, with
three alternating JVMs each. The shared class was 2-10% slower on most nested
shapes, most of all on array<int>, array<struct<int,string>>, array<array<int>>
and struct<int,string>, and level on flat ones. So I'd rather keep the three
copies. The differential tests cover each of them, as the three stale-view
mutations show, so a change that misses one of them fails.
The shared class did win on array<map<string,int>>, at 103 ns per row
against 120-178, but that turned out to be a separate C2 quirk. When C2 inlines
`MapWriter.writeEntries` into the loop over the array's maps, that loop is slow
under a mixed profile, and the shared class only avoided it because its
megamorphic call stops the inlining. `-XX:CompileCommand=dontinline` on
`writeEntries` also gives 103, and so does warming only that shape. `main`
takes 211 on it, so this isn't a regression, and I filed #6757 to dig into it.
--
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]
