feat: Create new `datafusion-comet-spark-expr` crate containing Spark-compatible DataFusion expressions #638

andygrove · 2024-07-07T15:52:02Z

Which issue does this PR close?

Part of #630

Also contains workaround for #642 that was discovered as part of this PR

Rationale for this change

This PR demonstrates how we can start moving expressions into a separate crate that other Rust projects can use, without having dependencies on JVM or Spark.

What changes are included in this PR?

New crate
Move abs expression to new crate as ther first example. If this approach is acceptable, we can follow up with PRs to move the other functions over
Added Rust tests for abs
Fall back to Spark for abs in legacy mode because behavior is incorrect (see abs implementation seems incorrect #642)

How are these changes tested?

Existing tests + new tests

andygrove · 2024-07-08T17:43:32Z

native/spark-expr/Cargo.toml

+# under the License.
+
+[package]
+name = "datafusion-comet-expr"


I wasn't sure whether to name this datafusion-spark-expr or datafusion-comet-expr. I think either could work.

I went with the latter because the versioning of this crate will be different from the version numbers used for other datafusion-* crates from the core project and thought that may cause confusion.

I also think it is unlikely that we would move this crate to the core project because we would want the implementation and tests to live in the same repo and the tests depend on Apache Spark, which would be a huge burden to add to the core project.

cc @alamb in case you have any opinions on this.

I think datafusion-comet-expr sounds good to me

You could get the best of both worlds like datafusion-comet-spark-expr but that is probably only useful if people end up using this crate outside the context of comet

Maybe if this crate really becomes a great set of spark compatible functions, that would be a good time to consider moving it to the core repo and releasing it along side the others

I prefer datafusion-spark-expr as it's a more general name so that the potential third party could find this crate more easily.

I agree that we may never move this crate to the core of the DataFusion project, but it's still possible we may want to move this crate to a datafusion-contrib repo once the supported expressions are stable and mature.

I have been thinking more about this, and I also now think we should name it datafusion-spark-expr because the functions are not specific to the Comet project. I will make that change.

I don't think we would ever want to move the code into datafusion-contrib because that is outside of Apache Software Foundation governance.

I don't think we would ever want to move the code into datafusion-contrib because that is outside of Apache Software Foundation governance.

Thanks for your explanation, I think it makes total sense and agree with that.

I like datafusion-comet-spark-expr fwiw. It would be nice for all things versioned together to have the sama datafusion-comet prefix, and as the reason to use these exprs over DF’s inbuilt one is to align with spark, having that in the name would make sense to me as well.

I have renamed the crate to datafusion-comet-spark-expr.

@advancedxy @viirya @huaxingao what do you think?

Looks okay to me.

Sounds good to me.

I have renamed the crate to datafusion-comet-spark-expr.

@advancedxy @viirya @huaxingao what do you think?

advancedxy · 2024-07-09T13:18:37Z

native/spark-expr/src/abs.rs

@@ -68,17 +71,15 @@ impl ScalarUDFImpl for CometAbsFunc {

    fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
        match self.inner_abs_func.invoke(args) {
-            Err(DataFusionError::ArrowError(ArrowError::ComputeError(msg), trace))
+            Err(DataFusionError::ArrowError(ArrowError::ComputeError(msg), _trace))


why this change, because _trace is never used?

We can simply change it to _?

Yes, if we get an overflow error then we don't care about the trace. I have renamed it to _.

advancedxy · 2024-07-09T13:24:41Z

native/spark-expr/Cargo.toml

+# under the License.
+
+[package]
+name = "datafusion-comet-expr"


I prefer datafusion-spark-expr as it's a more general name so that the potential third party could find this crate more easily.

I agree that we may never move this crate to the core of the DataFusion project, but it's still possible we may want to move this crate to a datafusion-contrib repo once the supported expressions are stable and mature.

advancedxy

LGTM. Pending CI passes.

BTW, I'm not familiar with the cargo related changes, so it would be great that others can take a look at that.

viirya · 2024-07-09T15:32:06Z

native/spark-expr/Cargo.toml

+datafusion-functions = { workspace = true }
+
+[lib]
+name = "datafusion_spark_expr"


Hmm, I wonder why we don't have comet in the library name? Are we planning to move these to datafusion? I personally prefer to keep spark specified stuffs in Comet.

There is a discussion about this at #638 (comment)

viirya · 2024-07-09T15:34:21Z

native/spark-expr/Cargo.toml

+
+[package]
+name = "datafusion-spark-expr"
+description = "DataFusion expressions that emulate Apache Spark's behavior"


Suggested change

description = "DataFusion expressions that emulate Apache Spark's behavior"

description = "DataFusion Comet expressions that emulate Apache Spark's behavior"

alamb

sorry forgot to submit my review

alamb · 2024-07-08T18:59:39Z

native/spark-expr/Cargo.toml

+# under the License.
+
+[package]
+name = "datafusion-comet-expr"


I think datafusion-comet-expr sounds good to me

You could get the best of both worlds like datafusion-comet-spark-expr but that is probably only useful if people end up using this crate outside the context of comet

Maybe if this crate really becomes a great set of spark compatible functions, that would be a good time to consider moving it to the core repo and releasing it along side the others

alamb · 2024-07-09T21:40:17Z

native/spark-expr/Cargo.toml

+datafusion-functions = { workspace = true }
+
+[lib]
+name = "datafusion_comet_spark_expr"


native/spark-expr/src/lib.rs

…-compatible DataFusion expressions (apache#638) * convert into workspace project * update GitHub actions * update Makefile * fix regression * update target path * update protobuf path in pom.xml * update more paths * add new datafusion-comet-expr crate * rename CometAbsFunc to Abs and add documentation * fix error message * improve error handling * update crate description * remove unused dep * address feedback * finish renaming crate * update README for datafusion-spark-expr * rename crate to datafusion-comet-spark-expr

andygrove added 12 commits July 6, 2024 06:50

convert into workspace project

520559d

update GitHub actions

1d39229

update Makefile

cb64234

fix regression

4617f8a

update target path

d033246

update protobuf path in pom.xml

91f1c0b

update more paths

e7fff73

add new datafusion-comet-expr crate

c900020

rename CometAbsFunc to Abs and add documentation

57ce6fd

fix error message

3caf372

improve error handling

b98cb34

update crate description

c2fb936

andygrove marked this pull request as ready for review July 8, 2024 15:56

andygrove mentioned this pull request Jul 8, 2024

abs implementation seems incorrect #642

Closed

andygrove commented Jul 8, 2024

View reviewed changes

upmerge

00751b0

andygrove force-pushed the spark-expr-crate branch from 0ed4d08 to 00751b0 Compare July 8, 2024 22:22

remove unused dep

33ce09a

andygrove requested review from kazuyukitanimura and huaxingao July 9, 2024 01:47

advancedxy reviewed Jul 9, 2024

View reviewed changes

address feedback

487ac57

andygrove changed the title ~~feat: Create new datafusion-comet-expr crate containing Spark-compatible DataFusion expressions~~ feat: Create new datafusion-spark-expr crate containing Spark-compatible DataFusion expressions Jul 9, 2024

andygrove added 2 commits July 9, 2024 08:17

finish renaming crate

7c093aa

update README for datafusion-spark-expr

dc665ab

advancedxy approved these changes Jul 9, 2024

View reviewed changes

viirya reviewed Jul 9, 2024

View reviewed changes

rename crate to datafusion-comet-spark-expr

8eec13e

andygrove changed the title ~~feat: Create new datafusion-spark-expr crate containing Spark-compatible DataFusion expressions~~ feat: Create new datafusion-comet-spark-expr crate containing Spark-compatible DataFusion expressions Jul 9, 2024

upmerge

4be923f

alamb reviewed Jul 9, 2024

View reviewed changes

native/spark-expr/Cargo.toml

datafusion-functions = { workspace = true }

[lib]

name = "datafusion_comet_spark_expr"

Copy link

Contributor

alamb Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

viirya reviewed Jul 10, 2024

View reviewed changes

native/spark-expr/src/lib.rs Show resolved Hide resolved

viirya approved these changes Jul 10, 2024

View reviewed changes

huaxingao approved these changes Jul 10, 2024

View reviewed changes

andygrove merged commit 10347b9 into apache:main Jul 10, 2024
73 checks passed

andygrove deleted the spark-expr-crate branch July 10, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create new `datafusion-comet-spark-expr` crate containing Spark-compatible DataFusion expressions #638

feat: Create new `datafusion-comet-spark-expr` crate containing Spark-compatible DataFusion expressions #638

andygrove commented Jul 7, 2024 •

edited

Loading

andygrove Jul 8, 2024 •

edited

Loading

alamb Jul 8, 2024

advancedxy Jul 9, 2024

andygrove Jul 9, 2024

advancedxy Jul 9, 2024

Blizzara Jul 9, 2024

andygrove Jul 9, 2024

viirya Jul 9, 2024

advancedxy Jul 9, 2024

huaxingao Jul 10, 2024

advancedxy Jul 9, 2024

andygrove Jul 9, 2024

advancedxy Jul 9, 2024

advancedxy left a comment

viirya Jul 9, 2024

andygrove Jul 9, 2024

viirya Jul 9, 2024

alamb left a comment

alamb Jul 8, 2024

alamb Jul 9, 2024

	description = "DataFusion expressions that emulate Apache Spark's behavior"
	description = "DataFusion Comet expressions that emulate Apache Spark's behavior"

feat: Create new datafusion-comet-spark-expr crate containing Spark-compatible DataFusion expressions #638

feat: Create new datafusion-comet-spark-expr crate containing Spark-compatible DataFusion expressions #638

Conversation

andygrove commented Jul 7, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

andygrove Jul 8, 2024 • edited Loading

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

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

advancedxy left a 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

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Create new `datafusion-comet-spark-expr` crate containing Spark-compatible DataFusion expressions #638

feat: Create new `datafusion-comet-spark-expr` crate containing Spark-compatible DataFusion expressions #638

andygrove commented Jul 7, 2024 •

edited

Loading

andygrove Jul 8, 2024 •

edited

Loading