chore: Use Datafusion's Sha2 and remove Comet's implementation. #2063
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2063 +/- ##
============================================
+ Coverage 56.12% 58.75% +2.62%
- Complexity 976 1253 +277
============================================
Files 119 137 +18
Lines 11743 13170 +1427
Branches 2251 2371 +120
============================================
+ Hits 6591 7738 +1147
- Misses 4012 4198 +186
- Partials 1140 1234 +94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Looks like the test-setup failed, tests were not run. |
Re-running. |
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for managing this task through DataFusion and back to Comet, @rishvin! Approved pending the CI pipeline that failed for an unrelated network issue.
| inputs: Seq[Attribute], | ||
| binding: Boolean): Option[ExprOuterClass.Expr] = { | ||
| if (!HashUtils.isSupportedType(expr)) { | ||
| return None |
There was a problem hiding this comment.
nit: It would be good to add a withInfo call to record the fallback reason.
There was a problem hiding this comment.
Thanks @andygrove for the review. We have withInfo inside the isSupportedType. Let me know if I'm missing something.
| // expression, however DataFusion does not support that yet. | ||
| val sha2Expr = expr.asInstanceOf[Sha2] | ||
| if (!sha2Expr.right.foldable) { | ||
| withInfo(expr, "For Sha2, non-foldable right argument is not supported") |
There was a problem hiding this comment.
The original info message was much more useful for the end user.
There was a problem hiding this comment.
Created #2193. Will fix this later this week.
There was a problem hiding this comment.
Opened a tiny PR to fix the message: #2213
Which issue does this PR close?
Closes #1820
Rationale for this change
Now that Comet's dependency has been updated to DF-49, the SHA2 fix apache/datafusion#16350 is available to Comet. This PR removes the Comet's SHA2 implementation and instead uses DataFusion's implementation.
What changes are included in this PR?
How are these changes tested?
hash functionstest case inCometExpressionSuite.