- 
                Notifications
    You must be signed in to change notification settings 
- Fork 247
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_ENABLEDconfig and remove references to that in the Spark diffs since it is no longer requiredAdd,Subtract,Multiply,Divide,IntegralDivide, andRemainder)AvgandSum)RoundCastspark.comet.expression.allowIncompatible, so thatsumandavgrun natively. This had the side effect of allowing more operators to run natively than before,How are these changes tested?