fix: Mark SortOrder with floating-point as incompatible #2650
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2650 +/- ##
============================================
+ Coverage 56.12% 59.20% +3.08%
- Complexity 976 1449 +473
============================================
Files 119 147 +28
Lines 11743 13755 +2012
Branches 2251 2365 +114
============================================
+ Hits 6591 8144 +1553
- Misses 4012 4387 +375
- Partials 1140 1224 +84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andygrove
changed the title
andygrove
changed the title
andygrove
changed the title
| withSQLConf(CometConf.getExprAllowIncompatConfigKey("SortOrder") -> "false") { | ||
| checkSparkAnswerAndFallbackReason( | ||
| "select * from tbl order by 1, 2", | ||
| "unsupported range partitioning sort order") |
There was a problem hiding this comment.
I filed a follow-up issue for improving the fallback reason to show the root cause
|
|
||
| override def getSupportLevel(expr: SortOrder): SupportLevel = { | ||
|
|
||
| def containsFloatingPoint(dt: DataType): Boolean = { |
There was a problem hiding this comment.
| def containsFloatingPoint(dt: DataType): Boolean = { | |
| def dataTypeContainsFloatingPoint(dt: DataType): Boolean = { |
There was a problem hiding this comment.
The method takes a DataType parameter, so it seems redundant to include that in the method name as well?
There was a problem hiding this comment.
Thanks @andygrove it is lgtm, just a small suggestion on naming
There was a problem hiding this comment.
I'll probably continue to grumble about what "incompatible" means but this LGTM. Thanks @andygrove!
4 participants
Which issue does this PR close?
Closes #2626
Rationale for this change
Fall back to Spark by default when sorting on floating-point values. Users can opt in if they don't care about the different handling of +0.0 vs -0.0.
What changes are included in this PR?
SortOrderHow are these changes tested?