-
Notifications
You must be signed in to change notification settings - Fork 2.5k
refactor: Fix rolling kernel dispatch with monotonic group attribute
#25494
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 #25494 +/- ##
==========================================
+ Coverage 79.39% 79.65% +0.25%
==========================================
Files 1742 1743 +1
Lines 240100 240295 +195
Branches 3038 3038
==========================================
+ Hits 190634 191408 +774
+ Misses 48683 48105 -578
+ Partials 783 782 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Now that we have these overlapping and monotonic attributes, setting them wrong somewhere can be disastrous. Can you replace all instances where we create a GroupsType with some sort of constructor (for example GroupStype::new_slice(groups, overlapping, monotonic)) which when #[cfg(debug_assertions)] is true actually checks those properties? That should allow us to catch bugs early in the CI.
Yes, for sure. That said, I’m just as concerned about the scenario where we set it correctly initially, but the groups later change and the attribute isn’t updated. We could put the attribute behind a getter and add run-time verification in debug builds, to be exercsied in CI and when debugging issues. These attributes aren’t accessed often and are typically used only for dispatch between different implementation paths. |
32634fb to
0636e3b
Compare
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.
Just a small nit then it's ready to go.
| false | ||
| } | ||
|
|
||
| assert!(!groups_overlap(&groups) || overlapping); |
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.
Swap the order, only do the test if it's claimed the groups aren't overlapping:
assert!(overlapping || !groups_overlap(&groups));You always want to put cheap conditions before expensive conditions if possible.
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.
Excellent point.
| } | ||
| true | ||
| } | ||
| assert!(groups_are_monotonic(&groups) || !monotonic); |
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 here.
fixes #25434
This also fixes an undocumented issue whereby the rolling group sum kernel returns
Noneon empty groups instead of0.This PR introduces the
monotonicattribute forgroups. It is used now -- in combination withoverlapping-- for dispatch to the rolling kernel, but it can be leveraged for other use cases in the future.The dispatch logic has been tested one-off for correct dispatch on
rollingandgroup_by_dynamic, and against false misclassifications on the CI suite. Still, it is possible that it gets misclassified now or later, but the impact is limited.Confirmed following the rolling agg kernel for sum:
group_by_dynamicrollingrollingingroup_bygroup_by_dynamicwithbyargumentThis PR replaces #25470