-
Notifications
You must be signed in to change notification settings - Fork 146
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
fix: Optimize not to call getNullCount as much as possible #820
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #820 +/- ##
=============================================
+ Coverage 33.94% 54.02% +20.07%
+ Complexity 874 854 -20
=============================================
Files 112 110 -2
Lines 42916 10559 -32357
Branches 9464 2000 -7464
=============================================
- Hits 14567 5704 -8863
+ Misses 25379 3857 -21522
+ Partials 2970 998 -1972 ☔ View full report in Codecov by Sentry. |
Somehow |
I am now running benchmarks with this PR |
I ran my local TPC-DS benchmark and it doesn't show any improvement for that benchmark. @kazuyukitanimura Do you see an improvement with any of the microbenchmark queries? |
Thanks @andygrove I used q27. |
This was the average of 3 runs for all 99 TPC-DS queries @ 100 GB scale factor |
@andygrove I ran q27 30 times. The improvement is not as big as I hoped but it is improving Before
After
|
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 @kazuyukitanimura
Thanks, I realized there might be a better way to do this. Please hold merging. |
Which issue does this PR close?
Rationale for this change
This PR reduces to call getNullCount as much as possible
What changes are included in this PR?
Calling getNullCount is expensive so reusing the number rather than re-calculating
How are these changes tested?
Existing test