andygrove commented on code in PR #6853:
URL: https://github.com/apache/datafusion-comet/pull/6853#discussion_r4376951544
##########
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:
Good catch, you're right: the read-back check runs after pruning, so it
can't catch this. In 93d8f7dbe6 `matchesReaderLayout` requires each vector to
hold exactly `batch.numRows()` values for the direct path, and anything else is
converted, which copies exactly `numRows` rows. The new test builds your
example (2 rows over vectors of 4, nulls in the last two) and checks it is not
written directly. It fails without the check.
--
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]
