mbutrovich commented on code in PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728#discussion_r3484638698
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -121,8 +121,7 @@ object CometSortArray extends
CometExpressionSerde[SortArray] with CodegenDispat
" floating-point types is not 100% compatible with Spark")
override def getUnsupportedReasons(): Seq[String] = Seq(
- "Nested arrays with `Struct` or `Null` child values are not supported
natively and will" +
- " fall back to Spark.")
+ "Nested arrays with `Struct` or `Null` child values are not supported
natively")
Review Comment:
`SortArray` actually has a second `Unsupported` branch, the
non-boolean-literal `ascendingOrder` case at line 156, and this PR routes that
through dispatch too. The description only calls out the nested `Struct`/`Null`
case. The dispatch path looks safe since Spark requires `ascendingOrder` to be
foldable so `doGenCode` can always compile it, but I do not see a test that
confirms this case stays native rather than falling back. Could we add one, or
confirm it is covered elsewhere?
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -121,8 +121,7 @@ object CometSortArray extends
CometExpressionSerde[SortArray] with CodegenDispat
" floating-point types is not 100% compatible with Spark")
override def getUnsupportedReasons(): Seq[String] = Seq(
- "Nested arrays with `Struct` or `Null` child values are not supported
natively and will" +
- " fall back to Spark.")
+ "Nested arrays with `Struct` or `Null` child values are not supported
natively")
Review Comment:
`SortArray` actually has a second `Unsupported` branch, the
non-boolean-literal `ascendingOrder` case at line 156, and this PR routes that
through dispatch too. The description only calls out the nested `Struct`/`Null`
case. The dispatch path looks safe since Spark requires `ascendingOrder` to be
foldable so `doGenCode` can always compile it, but I do not see a test that
confirms this case stays native rather than falling back. Could we add one, or
confirm it is covered elsewhere?
--
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]
