chore: Various improvements to checkSparkAnswer* methods in CometTestBase
#2656
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2656 +/- ##
============================================
+ Coverage 56.12% 59.21% +3.09%
- Complexity 976 1447 +471
============================================
Files 119 147 +28
Lines 11743 13759 +2016
Branches 2251 2365 +114
============================================
+ Hits 6591 8148 +1557
- 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
checkSparkAnswer* methods in CometTestBase [WIP]checkSparkAnswer* methods in CometTestBase [WIP]
andygrove
changed the title
checkSparkAnswer* methods in CometTestBase [WIP]checkSparkAnswer* methods in CometTestBase
| * A helper function for comparing Comet DataFrame with Spark result using absolute tolerance. | ||
| */ | ||
| protected def checkAnswerWithTol( | ||
| private def checkAnswerWithTol( |
There was a problem hiding this comment.
perhaps its good time to rename it for
checkAnswerWithTolerance
?
| " from int96timetbl" | ||
|
|
||
| if (conversionEnabled) { | ||
| // TODO this test should check for fallback reasons, but no fallback reason is recorded |
There was a problem hiding this comment.
does it mean it has to be an assertion on fallback reason?
There was a problem hiding this comment.
Yes, we should really assert that we are falling back to Spark for the expected reason, and not for some unrelated reason. No fallback reason is recorded in this case though, which is a bug IMO.
There was a problem hiding this comment.
Actually, the plugin is disabled if PARQUET_INT96_TIMESTAMP_CONVERSION is true. I updated the comment.
There was a problem hiding this comment.
This is a big improvement! Thanks for listening to me gripe about this and then doing something about it @andygrove!
|
Thanks for the review @mbutrovich and keep griping away! It leads to quality improvements. |
4 participants
Which issue does this PR close?
Closes #1313
Rationale for this change
What changes are included in this PR?
checkSparkAnswermethods now:checkAnswerWithTolnow supportsFloat(in addition toDouble)checkSparkAnswerAndCompareExplainPlanis removed and usages replaced withcheckSparkAnswerAndFallbackReason, which does not suffer from the bug described in Extended explain info missing fallback reason #1313checkSparkAnswerbut could be callingcheckSparkAnswerAndOperatorbecause the plan is fully nativecheckSparkAnswerbut could be callingcheckSparkAnswerAndFallbackReasonto check for a specific fallback reasonHow are these changes tested?