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 quantilesGK bug #58216
Fix quantilesGK bug #58216
Conversation
This is an automated comment for commit a89890b 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
|
Let's take a look at Fast Test. |
The code is not memory-safe: there is double-free: https://s3.amazonaws.com/clickhouse-test-reports/58216/d168dae7d1fca79903fe96f10f1711a349bacfb5/fuzzer_astfuzzerasan/report.html When you will fix it, also add the example from Fuzzer as a new test. |
I'm really confused about how to find the sql which caused crash. Need you help, thanks |
It should be near the end at |
@alexey-milovidov fixed, hope for your review and merge |
Ok. Looks perfect. Can we make a functional test for this? |
it was already added in tests/queries/0_stateless/02661_quantile_approx.sql. It is strange why it still complains @alexey-milovidov |
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.
The failures are unrelated. Please update with master so they go away.
And have a look at this comment. Let's add a sanity check so we don't reserve too much memory on corrupted input (which a client might send maliciously).
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.
Some comments about the PR:
- As a bugfix, this PR is bad. A whole refactor means that a) the PR is 100x what it needed to be (5 vs 500 lines), b) it takes longer to be reviewed and c) you get a full review and d) it won't be backported.
- I don't like moving code from .cpp to .h just to add an example that doesn't provide any value. For testing you can simply include the .cpp (
#include <AggregateFunctions/AggregateFunctionQuantileGK.cpp>
) and do whatever local tests you require. No need to add new headers that will be misused and grow the compilation and binary size. - I'd recommend you to clean up the commits before making a PR. Having commits adding prints or commenting and uncommenting code is bad as it will show you as the last person changing the code, although you left it exactly as it was.
Good advices. I'll make this PR clean. |
2a8f568
to
2797311
Compare
ba30493
to
a89890b
Compare
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix quantilesGK bug, close #57683