fix: Comet should not translate try_sum to native sum expression #277
Conversation
| case s @ Sum(child, _) if sumDataTypeSupported(s.dataType) => | ||
| case s @ Sum(child, evalMode) | ||
| if sumDataTypeSupported(s.dataType) && | ||
| evalMode == EvalMode.LEGACY => |
There was a problem hiding this comment.
Hmm, this is introduced in Spark 3.4. So Spark 3.2/3.3 builds are failed due to this change. I need to add a shim layer...
spark/src/main/scala/org/apache/comet/shims/ShimQueryPlanSerde.scala
Outdated
Show resolved
Hide resolved
| @@ -202,7 +202,7 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde { | |||
| inputs: Seq[Attribute], | |||
| binding: Boolean): Option[AggExpr] = { | |||
| aggExpr.aggregateFunction match { | |||
| case s @ Sum(child, _) if sumDataTypeSupported(s.dataType) => | |||
| case s @ Sum(child, _) if sumDataTypeSupported(s.dataType) && isLegacyMode(s) => | |||
There was a problem hiding this comment.
Not part of this PR.
But maybe we should add try_sum and try_xxx aggregation function support in the native side too?
There was a problem hiding this comment.
Yea, we should do it. We probably need to implement native try_sum expression because DataFusion might not have it.
There was a problem hiding this comment.
yeah, so I just went ahead and create an issue to track this one: #280.
….scala Co-authored-by: advancedxy <xianjin@apache.org>
|
There are some format issues that need to be fixed. |
|
I'd need one committer approval to merge this. 🙂 |
|
Thank you @sunchao 😄 |
|
Merged. Thanks @huaxingao @sunchao @advancedxy |
…che#277) * fix: Comet should not translate try_sum to native sum expression * For Spark 3.2 and 3.3 * Update spark/src/main/scala/org/apache/comet/shims/ShimQueryPlanSerde.scala Co-authored-by: advancedxy <xianjin@apache.org> * Fix format --------- Co-authored-by: advancedxy <xianjin@apache.org>
Which issue does this PR close?
Closes #276.
Rationale for this change
What changes are included in this PR?
How are these changes tested?