viirya commented on code in PR #6853:
URL: https://github.com/apache/datafusion-comet/pull/6853#discussion_r4273776044
##########
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:
I agree no producer hands the cache such a batch today, but I'm not sure the
read-back check covers it. Stats pruning runs before the read. Say a direct
batch reports 2 rows over vectors of 4, with the nulls in rows 2 and 3.
`numNulls` is then 2, which equals `numRows`, so the batch is recorded all null
and `IS NOT NULL` skips it without ever decoding it. The query then silently
loses rows where it used to fail. Could the direct path require that each
vector holds exactly `batch.numRows()` values, and convert otherwise?
`columnarBatchToArrowBatch` copies exactly `numRows` rows, so that case would
be written correctly instead of relying on the reader to catch it, which also
matches how `matchesReaderLayout` converts rather than trusts.
--
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]
