chore: Move all array_* serde to new framework, use correct INCOMPAT config #1349

andygrove · 2025-01-29T00:19:21Z

Which issue does this PR close?

Part of #1345

Rationale for this change

This PR updates the recently added array_* expression handling to mark them all as incompatible for now because we do not have comprehensive testing or validation for which data types are supported

The check is now based on a new COMET_EXPR_ALLOW_INCOMPATIBLE config instead of COMET_CAST_ALLOW_INCOMPATIBLE, which I had previously (mistakenly) suggested.

Rather than duplicate the if INCOMPAT check and the associated error message, there is now a new marker trait IncompatExpr.

What changes are included in this PR?

New IncompExpr trait
Moved logic for array_* serde into separate classes
Moved some tests from CometExpressionSuite to CometArrayExpressionSuite

How are these changes tested?

Existing tests

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

andygrove · 2025-01-29T00:35:45Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+      case _: ArrayRemove => toProto(CometArrayRemove)
+      case _: ArrayContains => toProto(CometArrayContains)
+      case _: ArrayAppend => toProto(CometArrayAppend)
+      case _: ArrayIntersect => toProto(CometArrayIntersect)
+      case _: ArrayJoin => toProto(CometArrayJoin)
+      case _: ArraysOverlap => toProto(CometArraysOverlap)


Eventually I would like to move this to a Map[SparkExpr, CometExpr]

andygrove · 2025-01-29T14:10:02Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

@@ -3490,3 +3437,6 @@ trait CometExpressionSerde {
      inputs: Seq[Attribute],
      binding: Boolean): Option[ExprOuterClass.Expr]
 }
+
+/** Marker trait for an expression that is not guaranteed to be 100% compatible with Spark */
+trait IncompatExpr {}


Later, we could add a method to this trait to provide the reason why an expression is incompatible and this could be used to generate documentation.

codecov-commenter · 2025-01-29T15:07:33Z

Codecov Report

Attention: Patch coverage is 70.42254% with 21 lines in your changes missing coverage. Please review.

Project coverage is 39.16%. Comparing base (f09f8af) to head (23accfa).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/scala/org/apache/comet/serde/arrays.scala	73.21%	13 Missing and 2 partials ⚠️
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	25.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #1349       +/-   ##
=============================================
- Coverage     56.12%   39.16%   -16.96%     
- Complexity      976     2065     +1089     
=============================================
  Files           119      262      +143     
  Lines         11743    60323    +48580     
  Branches       2251    12836    +10585     
=============================================
+ Hits           6591    23627    +17036     
- Misses         4012    32223    +28211     
- Partials       1140     4473     +3333

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

comphead · 2025-01-29T16:13:20Z

common/src/main/scala/org/apache/comet/CometConf.scala

+    conf("spark.comet.expression.allowIncompatible")
+      .doc(
+        "Comet is not currently fully compatible with Spark for all expressions. " +
+          "Set this config to true to allow them anyway. See compatibility guide " +


Suggested change

"Set this config to true to allow them anyway. See compatibility guide " +

"Set this config to true to allow them anyway. See Compatibility Guide " +

comphead · 2025-01-29T16:14:00Z

common/src/main/scala/org/apache/comet/CometConf.scala

+      .doc(
+        "Comet is not currently fully compatible with Spark for all expressions. " +
+          "Set this config to true to allow them anyway. See compatibility guide " +
+          "for more information.")


Suggested change

"for more information.")

"for more information. (docs/user-guide/compatibility.md)")

I pushed a commit to update this by referencing a new CometConf.COMPAT_GUIDE variable with the link to the published documentation.

I regenerated the compatibility guide as well.

comphead · 2025-01-29T16:14:34Z

docs/source/user-guide/configs.md

@@ -64,6 +64,7 @@ Comet provides the following configuration settings.
 | spark.comet.explain.native.enabled | When this setting is enabled, Comet will provide a tree representation of the native query plan before execution and again after execution, with metrics. | false |
 | spark.comet.explain.verbose.enabled | When this setting is enabled, Comet will provide a verbose tree representation of the extended information. | false |
 | spark.comet.explainFallback.enabled | When this setting is enabled, Comet will provide logging explaining the reason(s) why a query stage cannot be executed natively. Set this to false to reduce the amount of logging. | false |
+| spark.comet.expression.allowIncompatible | Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. See compatibility guide for more information. | false |


Suggested change

| spark.comet.expression.allowIncompatible | Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. See compatibility guide for more information. | false |

| spark.comet.expression.allowIncompatible | Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. See Compatibility Guide for more information. (docs/user-guide/compatibility.md) | false |

comphead · 2025-01-29T16:16:43Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+        case _: IncompatExpr if !CometConf.COMET_EXPR_ALLOW_INCOMPATIBLE.get() =>
+          withInfo(
+            expr,
+            s"$expr is not fully compatible with Spark. To enable it anyway, set " +


Suggested change

s"$expr is not fully compatible with Spark. To enable it anyway, set " +

s"$expr is not fully compatible with Spark, see details in Compatibility Guide. To enable it anyway, set " +

I pushed a commit to update this by referencing a new CometConf.COMPAT_GUIDE variable with the link to the published documentation.

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

Co-authored-by: Oleks V <[email protected]>

…t into array-incompat

kazuyukitanimura · 2025-01-29T19:16:45Z

docs/source/user-guide/configs.md

@@ -64,6 +64,7 @@ Comet provides the following configuration settings.
 | spark.comet.explain.native.enabled | When this setting is enabled, Comet will provide a tree representation of the native query plan before execution and again after execution, with metrics. | false |
 | spark.comet.explain.verbose.enabled | When this setting is enabled, Comet will provide a verbose tree representation of the extended information. | false |
 | spark.comet.explainFallback.enabled | When this setting is enabled, Comet will provide logging explaining the reason(s) why a query stage cannot be executed natively. Set this to false to reduce the amount of logging. | false |
+| spark.comet.expression.allowIncompatible | Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. For more information, refer to the Comet Compatibility Guide (https://datafusion.apache.org/comet/user-guide/compatibility.html). | false |


Not a blocker but looks like the compatibility guide does not mention about the array related issues

Thanks. I had updated this locally but forgot that these changes have to go in compatibility-template.md instead of compatibility,md. 🤦‍♂️

I have added them now.

kazuyukitanimura

pending ci

parthchandra

lgtm

fix

2d97a57

andygrove requested a review from kazuyukitanimura January 29, 2025 00:23

andygrove marked this pull request as ready for review January 29, 2025 00:23

rename config

7f00ab9

andygrove commented Jan 29, 2025

View reviewed changes

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala Outdated Show resolved Hide resolved

andygrove commented Jan 29, 2025

View reviewed changes

andygrove added 2 commits January 28, 2025 17:43

fix

6460add

fix

5c333b7

andygrove commented Jan 29, 2025

View reviewed changes

andygrove requested a review from comphead January 29, 2025 14:18

comphead reviewed Jan 29, 2025

View reviewed changes

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala Show resolved Hide resolved

andygrove and others added 4 commits January 29, 2025 10:42

Update spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

1c5e992

Co-authored-by: Oleks V <[email protected]>

address feedback

a3e9c46

Merge branch 'array-incompat' of github.com:andygrove/datafusion-come…

d44bb0e

…t into array-incompat

address feedback

d6fa542

kazuyukitanimura approved these changes Jan 29, 2025

View reviewed changes

kazuyukitanimura reviewed Jan 29, 2025

View reviewed changes

andygrove added 2 commits January 29, 2025 14:01

Update documentation

e523871

scalastyle

23accfa

parthchandra approved these changes Jan 29, 2025

View reviewed changes

andygrove merged commit 443b5db into apache:main Jan 30, 2025
74 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Move all array_* serde to new framework, use correct INCOMPAT config #1349

chore: Move all array_* serde to new framework, use correct INCOMPAT config #1349

andygrove commented Jan 29, 2025 •

edited

Loading

andygrove Jan 29, 2025

andygrove Jan 29, 2025

codecov-commenter commented Jan 29, 2025 •

edited

Loading

comphead Jan 29, 2025

comphead Jan 29, 2025

andygrove Jan 29, 2025

comphead Jan 29, 2025

comphead Jan 29, 2025

andygrove Jan 29, 2025

kazuyukitanimura Jan 29, 2025

andygrove Jan 29, 2025

kazuyukitanimura left a comment

parthchandra left a comment

	"Set this config to true to allow them anyway. See compatibility guide " +
	"Set this config to true to allow them anyway. See Compatibility Guide " +

	"for more information.")
	"for more information. (docs/user-guide/compatibility.md)")

	\| spark.comet.expression.allowIncompatible \| Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. See compatibility guide for more information. \| false \|
	\| spark.comet.expression.allowIncompatible \| Comet is not currently fully compatible with Spark for all expressions. Set this config to true to allow them anyway. See Compatibility Guide for more information. (docs/user-guide/compatibility.md) \| false \|

	s"$expr is not fully compatible with Spark. To enable it anyway, set " +
	s"$expr is not fully compatible with Spark, see details in Compatibility Guide. To enable it anyway, set " +

chore: Move all array_* serde to new framework, use correct INCOMPAT config #1349

chore: Move all array_* serde to new framework, use correct INCOMPAT config #1349

Conversation

andygrove commented Jan 29, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura left a comment

Choose a reason for hiding this comment

parthchandra left a comment

Choose a reason for hiding this comment

andygrove commented Jan 29, 2025 •

edited

Loading

codecov-commenter commented Jan 29, 2025 •

edited

Loading