Feat: support array_compact function #1321
Conversation
# Conflicts: # native/core/src/execution/planner.rs # native/proto/src/proto/expr.proto # spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala # spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
# Conflicts: # native/core/src/execution/planner.rs # native/proto/src/proto/expr.proto # spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala # spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
kazantsev-maksim
changed the title
kazantsev-maksim
changed the title
# Conflicts: # spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
Outdated
Show resolved
Hide resolved
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
Outdated
Show resolved
Hide resolved
# Conflicts: # native/core/src/execution/planner.rs # native/proto/src/proto/expr.proto # spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala # spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
=============================================
- Coverage 56.12% 39.53% -16.60%
- Complexity 976 2087 +1111
=============================================
Files 119 265 +146
Lines 11743 61668 +49925
Branches 2251 13114 +10863
=============================================
+ Hits 6591 24379 +17788
- Misses 4012 32723 +28711
- Partials 1140 4566 +3426 ☔ View full report in Codecov by Sentry. |
| @@ -830,6 +830,25 @@ impl PhysicalPlanner { | |||
| )); | |||
| Ok(array_has_any_expr) | |||
| } | |||
| ExprStruct::ArrayCompact(expr) => { | |||
There was a problem hiding this comment.
Wondering if we can use https://github.com/apache/datafusion-comet/blob/main/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala#L2116
scalarExprToProtoWithReturnType or scalarExprToProto
so that we do not have to define here for every Array related function
There was a problem hiding this comment.
I can try to do it, but is it necessary to do it as part of this issue?
There was a problem hiding this comment.
Up to you. If you would like to work on it separately, that's ok with me.
I filed #1459
If you prefer, we can merge this first, but I feel we should clean up first before adding next array related functions
Which issue does this PR close?
Related to Epic: #1042
array_compact: SELECT array_compact(array(1, 2, 3, null)) => array(1, 2, 3)
DataFusion' s array_compact has same behavior with Spark 's array_compact function
Spark: https://docs.databricks.com/en/sql/language-manual/functions/array_compact.html
DataFusion: https://datafusion.apache.org/user-guide/sql/scalar_functions.html#array-remove-all
Rationale for this change
Defined under Epic: #1042
What changes are included in this PR?
planner.rs: Maps Spark 's arrays_compact function to DataFusion array_remove_all_udf physical expression from Spark physical expression
expr.proto: arrays_compact message has been added,
QueryPlanSerde.scala: arrays_compact pattern matching case has been added,
CometExpressionSuite.scala: A new UT has been added for arrays_compact function.
How are these changes tested?
A new UT has been added.