-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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 queries with FINAL give wrong result when table does not use adaptive granularity #62432
Fix queries with FINAL give wrong result when table does not use adaptive granularity #62432
Conversation
… non-intersect part Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
INSERT INTO account_test | ||
SELECT * FROM (SELECT * FROM generateRandom('id UInt64, row_ver UInt64',1234) LIMIT 1000) WHERE row_ver > 14098131981223776000; | ||
|
||
SELECT 'GOOD', * FROM account_test FINAL WHERE id = 11338881281426660955 SETTINGS split_parts_ranges_into_intersecting_and_non_intersecting_final = 1; |
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.
It is better to add another query in test:
SELECT 'GOOD', * FROM account_test FINAL WHERE id = 11338881281426660955 SETTINGS split_parts_ranges_into_intersecting_and_non_intersecting_final = 0;
So it is easier to compare outputs.
This is an automated comment for commit 6f4a8bf 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
|
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
Try fixing CI
AST Fuzzer test is failing, but I think it doesn't relates to the change this PR. Can reproduce in upstream Update: even without |
|
Stress test failure relates to database replicated #62742 |
Ping @Algunenano I think this PR is ready to merge |
@canhld94, what is with these logical errors detected by our fuzzers? |
@alexey-milovidov this is a known bug, I opened an issue for it #62741 and have an attempt to fix in #62776 but not yet found a good approach. |
Sorry for the delay reviewing this. Initially it slipped through the cracks and then I got sidetracked by bugs in related code (but not in this PR). I've simplified the test so it doesn't depend on generateRandom and updated with master to make sure things are still ok. |
Thanks! Nevertheless, the AST fuzzer failure is still there, not causing by this PR but merging this PR will reproduce the failure in master. May be we need to push to fix this bug soon. |
e6880e9
…13a2bda5bd5df1a508c2e7168e9e6 Cherry pick #62432 to 24.2: Fix queries with FINAL give wrong result when table does not use adaptive granularity
…n table does not use adaptive granularity
…13a2bda5bd5df1a508c2e7168e9e6 Cherry pick #62432 to 24.3: Fix queries with FINAL give wrong result when table does not use adaptive granularity
…n table does not use adaptive granularity
…13a2bda5bd5df1a508c2e7168e9e6 Cherry pick #62432 to 24.4: Fix queries with FINAL give wrong result when table does not use adaptive granularity
…n table does not use adaptive granularity
Backport #62432 to 24.3: Fix queries with FINAL give wrong result when table does not use adaptive granularity
Backport #62432 to 24.4: Fix queries with FINAL give wrong result when table does not use adaptive granularity
Backport #62432 to 24.2: Fix queries with FINAL give wrong result when table does not use adaptive granularity
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Close #62426
Modify your CI run:
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Include tests (required builds will be added automatically):
Exclude tests:
Extra options:
Only specified batches in multi-batch jobs: