feat: Support murmur3_hash and sha2 family hash functions by advancedxy · Pull Request #226 · apache/datafusion-comet

advancedxy · 2024-03-22T06:53:29Z

Which issue does this PR close?

This partially closes #205

Rationale for this change

More expression coverage for comet

What changes are included in this PR?

add user_defined scalar function: spark_murmur3_hash to support murmur3_hash in comet
refine and re-enable md5 expression test.
bridge sha2 family functions to Datafusion's built-in scalar functions
glue code to convert Spark expression to proto messages
new test code

How are these changes tested?

Added new test code

core/src/execution/datafusion/spark_hash.rs

codecov-commenter · 2024-03-22T07:53:48Z

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 33.35%. Comparing base (8aab44c) to head (1c63018).
Report is 5 commits behind head on main.

Files	Patch %	Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	90.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #226      +/-   ##
============================================
- Coverage     33.41%   33.35%   -0.06%     
- Complexity      768      770       +2     
============================================
  Files           107      107              
  Lines         36329    37057     +728     
  Branches       7935     8110     +175     
============================================
+ Hits          12138    12361     +223     
- Misses        21643    22097     +454     
- Partials       2548     2599      +51

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

advancedxy · 2024-03-22T08:20:45Z

All cleared, cc @sunchao @viirya PTAL.

core/src/execution/datafusion/spark_hash.rs

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

advancedxy · 2024-03-22T15:06:20Z

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


        case Md5(child) =>
-          val childExpr = exprToProtoInternal(Cast(child, StringType), inputs)
+          val childExpr = exprToProtoInternal(child, inputs)


I don't think we need to add a cast here? As Spark will perform the cast to Binary type and DataFusion supports both utf8 and binary input now.

Is binary support added to DataFusion Md5 recently?

hmmm, I think binary support is added by this PR: apache/datafusion#3124, which is pretty old.

Anyway, currently all the digest func supports both utf8 and binary input.

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

advancedxy · 2024-04-10T15:19:58Z

I have rebased again the latest main branch. cc @sunchao @viirya PTAL.

advancedxy · 2024-04-17T02:54:04Z

Gently ping @sunchao and @viirya.

viirya · 2024-04-17T05:57:55Z

I will review this soon. Thanks.

viirya · 2024-04-23T05:35:32Z

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala


-  // TODO: enable this when we add md5 function to Comet
-  ignore("md5") {
+  test("md5") {


Hmm, I remember we explicitly disable it because DataFusion crypto_expressions feature includes blake3 which cannot be built on Mac platform.

I don't see you add crypto_expressions to Cargo.toml, how does it work?

hmm, I think crypto_expressions is enabled as default features in DataFusion?

I remember we explicitly disable it because DataFusion crypto_expressions feature includes blake3 which cannot be built on Mac platform.

Do you have any issues to track this one? I think I can build DataFusion by default on Apple Silicon.

hmm, I think crypto_expressions is enabled as default features in DataFusion?

Yes, but we don't use default features:

default-features = false

I remember we explicitly disable it because DataFusion crypto_expressions feature includes blake3 which cannot be built on Mac platform.

Do you have any issues to track this one? I think I can build DataFusion by default on Apple Silicon.

Not sure if the crate is updated to fix that. We encountered the issue and disabled crypto_expressions one year ago (internally, before we open sourced Comet).

Maybe it is okay now. But I'm wondering as we don't add back crypto_expressions feature, is md5 function working? I think it is guarded by this feature in DataFusion.

But I'm wondering as we don't add back crypto_expressions feature, is md5 function working? I think it is guarded by this feature in DataFusion.

That's new.
I did a quick digging. It seems that the crypto expressions are enabled in datafusion-functions as it's enabled as default features.

Let me ensure the crypto_expressions feature is enabled then.

is md5 function working?

I manually checked with the web ui and the unit test passes:

core/src/execution/datafusion/expressions/scalar_funcs.rs

viirya · 2024-04-23T05:56:23Z

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

          }

+        case Murmur3Hash(children, seed) if children.forall(c => supportedDataType(c.dataType)) =>
+          // TODO: support list/map/struct type for murmur3 hash


This TODO seems unnecessary as other expressions also don't support nested types.

I see.
Let me create an issue to track the complex(list/map/struct) type support then? I think we can start by supporting them in literal and scalar expressions.

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

advancedxy · 2024-04-24T07:26:28Z

@viirya do you have any other comments?

viirya · 2024-04-24T13:28:57Z

I will take another look today.

viirya · 2024-04-26T05:02:54Z

Merged. Thanks.

advancedxy · 2024-04-26T10:03:17Z

Merged. Thanks.

Thanks for your comments and review.

* feat: Support murmur3_hash and sha2 family hash functions * address comments * apply scalafix * ensure crypto_expressions feature is enabled

advancedxy commented Mar 22, 2024

View reviewed changes

core/src/execution/datafusion/spark_hash.rs Outdated Show resolved Hide resolved

viirya reviewed Mar 22, 2024

View reviewed changes

core/src/execution/datafusion/spark_hash.rs Outdated Show resolved Hide resolved

viirya reviewed Mar 22, 2024

View reviewed changes

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

advancedxy commented Mar 22, 2024

View reviewed changes

advancedxy changed the title ~~feat: Support murmur3_hash, md5 and sha2 hash functions~~ feat: Support murmur3_hash and sha2 family hash functions Mar 22, 2024

advancedxy commented Mar 22, 2024

View reviewed changes

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

advancedxy mentioned this pull request Apr 7, 2024

fix: Remove wrong calculation for Murmur3Hash for float with null input #245

Merged

advancedxy force-pushed the hash_functions branch from 1c63018 to e55f8fc Compare April 10, 2024 12:00

advancedxy mentioned this pull request Apr 15, 2024

feat: Add manual test to calculate spark builtin functions coverage #263

Merged

advancedxy force-pushed the hash_functions branch from e55f8fc to 8931c10 Compare April 17, 2024 02:51

viirya reviewed Apr 23, 2024

View reviewed changes

core/src/execution/datafusion/expressions/scalar_funcs.rs Outdated Show resolved Hide resolved

viirya reviewed Apr 23, 2024

View reviewed changes

core/src/execution/datafusion/expressions/scalar_funcs.rs Outdated Show resolved Hide resolved

viirya reviewed Apr 23, 2024

View reviewed changes

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

advancedxy added 2 commits April 23, 2024 21:48

feat: Support murmur3_hash and sha2 family hash functions

5d63ffc

address comments

991fec0

advancedxy force-pushed the hash_functions branch from 8931c10 to 991fec0 Compare April 23, 2024 14:05

advancedxy added 2 commits April 23, 2024 22:16

apply scalafix

004493a

ensure crypto_expressions feature is enabled

bc97c1a

viirya approved these changes Apr 26, 2024

View reviewed changes

viirya merged commit 8485558 into apache:main Apr 26, 2024

Comments

Conversation

advancedxy commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Uh oh!

codecov-commenter commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

advancedxy commented Mar 22, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

advancedxy commented Apr 10, 2024

Uh oh!

advancedxy commented Apr 17, 2024

Uh oh!

viirya commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

advancedxy commented Apr 24, 2024

Uh oh!

viirya commented Apr 24, 2024

Uh oh!

viirya commented Apr 26, 2024

Uh oh!

advancedxy commented Apr 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

advancedxy commented Mar 22, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited

Loading