andygrove commented on PR #5526:
URL:
https://github.com/apache/datafusion-comet/pull/5526#issuecomment-5876614088
This is a light fully automated review since there are so many PRs open.
Following up on my earlier compatibility-page comment: four serdes whose
support level moved in this PR still return an empty `getUnsupportedReasons()`,
so `GenerateDocs` will not list their new restriction. `CometSize`
(`spark/src/main/scala/org/apache/comet/serde/arrays.scala:931`) and
`CometArrayAppend` (`arrays.scala:57`) now return
`Unsupported(NullGuard.reason)` for a non-deterministic child.
`CometCollectSet` and `CometCollectList`
(`spark/src/main/scala/org/apache/comet/serde/aggregates.scala:1056` and
`:1132`) now return `Unsupported` through
`CometCollectAggregate.nestedNullLevel` for any `NullType`-bearing input. For
example, on Spark 4.x, where `legacySizeOfNull` is off by default,
`size(filter(arr, x -> x > rand()))` over a nullable `arr` now falls back to
Spark, but the `size` entry on the generated page won't mention it.
`CometArraysZip` and `CometMapFromArrays` already publish `NullGuard.reason`,
so the same override would cover `size` and `array_append`. Hoisting
the collect reason into a `val` would let both aggregates publish it too. The
gate check in `CometNullTypeCompositionSuite` ("NullType gates enroll in the
JVM codegen dispatcher and publish their reasons") lists `arrays_zip` and
`map_from_arrays` but none of these four, which is why it passed. Could you add
them there as well? The collect ones would need a lookup through `aggrSerdeMap`
rather than `exprSerdeMap`.
--
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]
