peterxcli commented on code in PR #6853:
URL: https://github.com/apache/datafusion-comet/pull/6853#discussion_r4264910656
##########
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:
Any chance we could completely get rid of this?
suggestion is: Gather the stats after the conversion in encodeBatches, from
arrowBatch in the convert branch and from batch in the direct branch. Two
things make this work:
1. Every `CometVector` subclass in the repo extends `CometDecodedVector`:
Plain, Dictionary, Struct, List and Map. The only exception is the private
`IntervalChildVector`, which never appears as a top-level column.
2. `isArrowBacked` lets only `CometVectors` through to the direct path.
After that change, every batch the collector sees has an Arrow null count,
and ArrowWriter has already made the row pass while building it. countNulls and
the comment about Spark’s Parquet reader could go,
##########
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:
the comparison - `numValues() == numRows` here maybe is too defensive, looks
like we dont need this guard
--
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]
