andygrove commented on PR #5603:
URL:
https://github.com/apache/datafusion-comet/pull/5603#issuecomment-5876628283
This is a light fully automated review since there are so many PRs open.
The `RowArrowReader`, `SparkColumnarArrowReader` and `CometArrowConverters`
changes in 714aa8d9b (`RowArrowReader.scala:58`,
`SparkColumnarArrowReader.scala:55`, `CometArrowConverters.scala:69` and `:99`)
are the fix for the cached-view panic from my last review, but I don't think
any test reaches them with a duplicate-name struct. The new case at
`CometInMemoryCacheSuite.scala:165` builds `named_struct('x', id, 'x', id + 1)`
in a native `CometProject` above `CometSparkRowToColumnar(Range)`, so
`RowArrowReader` only ever sees `id`. The cache is then written from
Arrow-backed batches through `serializeBatchColumns` rather than
`rowToArrowBatchIter`, and read back through `CometArrowStreamReader` and
`ColumnarBatchArrowReader`, which already handled duplicates before that
commit. As far as I can trace it, the test would pass with all three changes
reverted. Could you add the `cacheTable` repro from my last review as a test in
a suite that keeps Spark's default cache serializer, wher
e `SELECT s FROM v WHERE k > 1` plans `CometFilter` over
`CometSparkRowToColumnar`? A `CometInMemoryCacheSuite` case where Spark builds
the struct would cover the converter, for example caching with
`spark.comet.sparkToColumnar.enabled=false` so the cached plan is a Spark
`Project` and `convertInternalRowToCachedBatch` runs. These would also show
whether reconciling main's duplicate-name guards really lets these shapes
through, since main's `ArrowCachedBatchSerializer.supportsType` now declines
duplicate names too (#6004), and with that check in place the new cache test
would not plan `CometInMemoryTableScan` at all.
--
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]
