-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
ArgMin / ArgMax / any / anyLast / anyHeavy optimization #58640
Conversation
This is an automated comment for commit 7e2b5ef with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
Constructor should be called by init. Let's verify
After multiple iterations and revisions by me I think the code is ready for review. Right now there's only some slight degradation in some JIT cases (but others are similarly improved and I'm not sure why), big improvements in argMin / argMax as well as LIMIT 1, and big improvements in compilation times. |
This is ready for review, if anybody wants to have a look. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
ORDER BY {u8/u16/u32/u64/i8/i16/u32/i64) LIMIT 1
queries.Documentation entry for user-facing changes
Multiple improvements:
AggregateFunctionsSingleValue
base class and its changeIfBetter API, which was terrible. Having different classes built directly on top of data allows to implement batch APIs and make use of SIMD functions.SingleValueDataBase
from which otherSingleValue*
classes inherit. This introduces some extra memory usage (for the virtual pointer) but allows to use base classes and compile derived classes only once. As a result ArgMax/ArgMin compilation does not need double template instantiation (builds in ~95% less time). Same for AnyHeavy or SingleValueOrNullORDER BY {u8/u16/u32/u64/i8/i16/u32/i64) LIMIT 1
queries (argMin is permutation with LIMIT 1).