-
Notifications
You must be signed in to change notification settings - Fork 246
feat: Improve fallback mechanism for ANSI mode #2211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2211 +/- ##
============================================
+ Coverage 56.12% 57.97% +1.84%
- Complexity 976 1275 +299
============================================
Files 119 143 +24
Lines 11743 13312 +1569
Branches 2251 2375 +124
============================================
+ Hits 6591 7717 +1126
- Misses 4012 4344 +332
- Partials 1140 1251 +111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is great @andygrove. Adding a comment to link PR #2136 which is created to implement ANSI mode for arithmetic operations. |
adf3292
to
d72a4b5
Compare
278c26b
to
a6afecb
Compare
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
Outdated
Show resolved
Hide resolved
+ if (!isCometEnabled) { | ||
+ // Comet's error message does not include the original SQL query | ||
+ // https://github.com/apache/datafusion-comet/issues/2215 | ||
+ assert(msg.contains(query)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When ANSI was not enabled, was this passing?
If so, is there a way to check whether ANSI is enabled and skip only when both comet and ansi are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were previously falling back to Spark for this test. This Spark test is specifically for testing ANSI errors from cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall now why we were falling back to Spark so I will take another look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Spark 3.x we previously fell back to Spark for this test because we do not run the Spark SQL tests for 3.x with ENABLE_COMET_ANSI_MODE=true
and therefore did not enable spark.comet.ansi.enabled
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andygrove. I think we should have a bigger discussion about defaults and tuning suggestions for ANSI mode.
Which issue does this PR close?
Closes #1974
Part of #313
Rationale for this change
Rather than fall back for all queries when ANSI mode is enabled, just fall back if the query is using expressions that are affected by ANSI mode.
What changes are included in this PR?
COMET_ANSI_MODE_ENABLED
config and remove references to that in the Spark diffs since it is no longer requiredAdd
,Subtract
,Multiply
,Divide
,IntegralDivide
, andRemainder
)Avg
andSum
)Round
Cast
spark.comet.expression.allowIncompatible
, so thatsum
andavg
run natively. This had the side effect of allowing more operators to run natively than before,How are these changes tested?