-
Notifications
You must be signed in to change notification settings - Fork 182
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
feat: Add aggregate expression fuzz testing in CI #1374
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1374 +/- ##
=============================================
- Coverage 56.12% 39.17% -16.95%
- Complexity 976 2065 +1089
=============================================
Files 119 262 +143
Lines 11743 60327 +48584
Branches 2251 12836 +10585
=============================================
+ Hits 6591 23635 +17044
- Misses 4012 32221 +28209
- Partials 1140 4471 +3331 ☔ View full report in Codecov by Sentry. |
// ok | ||
} else { | ||
|
||
def almostEqual(a: Double, b: Double, tolerance: Double = 1e-6f): Boolean = { |
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.
Looks like there are multiple identical definitions of almostEqual
?
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.
Yes, the code is hacky and experimental. Will clean up before marking as ready for review.
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.
Actually, the two implementations of almostEqual
have different input types (float vs double).
// ok | ||
} else { | ||
|
||
def almostEqual(a: Double, b: Double, tolerance: Double = 1e-6f): Boolean = { |
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.
Same implementation as above
|
||
val scanTypes = Seq( | ||
CometConf.SCAN_NATIVE_COMET | ||
/*CometConf.SCAN_NATIVE_DATAFUSION, |
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.
Do we plan to have these scan implementations also added to the fuzz testing at some point?
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.
Yes. These are commented out for now because they fail due to timestamp issues.
Which issue does this PR close?
Part of #1345
Rationale for this change
We want to add randomized testing to help prevent regressions and increase test coverage.
What changes are included in this PR?
How are these changes tested?
New test, enabled in CI.