andygrove commented on code in PR #6853:
URL: https://github.com/apache/datafusion-comet/pull/6853#discussion_r4272170030
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowCachedBatchSerializer.scala:
##########
@@ -163,6 +160,29 @@ class ArrowCachedBatchSerializer extends
SimpleMetricsCachedBatchSerializer {
(lower, upper, nulls)
}
+ // The column's null count if its Arrow vector already holds one, or -1 if
only a pass over the
+ // rows can tell.
+ //
+ // A CometDecodedVector takes its null count from Arrow once, when it is
built, and its isNullAt
+ // answers from the same validity bitmap, so the two agree whenever the
vector holds exactly the
+ // batch's rows. Spark's own vectors do not qualify: its vectorized Parquet
reader marks a column
+ // missing from the file all-null, and that vector's numNulls is then 0
while isNullAt is true
+ // for every row.
+ private def arrowNullCount(col: ColumnVector, numRows: Int): Int = col match
{
+ case v: CometDecodedVector if v.numValues() == numRows => v.numNulls()
+ case _ => -1
+ }
+
+ private def countNulls(col: ColumnVector, numRows: Int): Int = {
+ var nullCount = 0
+ var r = 0
+ while (r < numRows) {
+ if (col.isNullAt(r)) nullCount += 1
+ r += 1
+ }
+ nullCount
+ }
Review Comment:
Thanks, that's a better structure. Done in b979e1b07e: `encodeBatches`
converts a batch that isn't written directly first and gathers the statistics
from the batch it serializes, so the collector only ever sees
`CometDecodedVector`s. `countNulls` is gone, and the collector now throws if it
is handed any other vector. I kept one sentence about Spark's Parquet reader in
`encodeBatches`, since it's the reason the input's `numNulls` can't be used,
and the missing-column test still guards it.
I measured it first, because Arrow's getters had looked slower than Spark's
for strings and wide decimals in the synthetic benchmark. On real batches from
Spark's vectorized Parquet reader the converted batch is faster to read: over
5M rows and ten columns, the statistics took 385 to 409 ms, against 493 to 516
ms from Spark's vectors. Strings and `decimal(38,2)` gained the most, plain
bigint and double moved by under a millisecond, and the conversion costs the
same either way. The description has the per-column numbers.
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowCachedBatchSerializer.scala:
##########
@@ -163,6 +160,29 @@ class ArrowCachedBatchSerializer extends
SimpleMetricsCachedBatchSerializer {
(lower, upper, nulls)
}
+ // The column's null count if its Arrow vector already holds one, or -1 if
only a pass over the
+ // rows can tell.
+ //
+ // A CometDecodedVector takes its null count from Arrow once, when it is
built, and its isNullAt
+ // answers from the same validity bitmap, so the two agree whenever the
vector holds exactly the
+ // batch's rows. Spark's own vectors do not qualify: its vectorized Parquet
reader marks a column
+ // missing from the file all-null, and that vector's numNulls is then 0
while isNullAt is true
+ // for every row.
+ private def arrowNullCount(col: ColumnVector, numRows: Int): Int = col match
{
+ case v: CometDecodedVector if v.numValues() == numRows => v.numNulls()
Review Comment:
You're right that it isn't needed once the statistics come from the written
batch, so b979e1b07e drops it along with the test that pinned it. A converted
batch holds exactly the input's rows. A batch written directly has to hold
exactly its rows anyway, because `serialize` takes the payload's row count from
the first vector, so a batch reporting fewer rows than its vectors would fail
when read back.
--
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]
