-
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
add setting exact_rows_before_limit #25333
add setting exact_rows_before_limit #25333
Conversation
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.
Changelog entry (and the in-code description) is obscure for users: you should describe the new setting at the user-level scenarios - since typical user doesn't know what is LimitBlockInputStream
, where it's located in the pipeline and how it affects typical SELECT
queries.
Also please add the test.
Hi abyss7 ,thank you for your quick reply. I have edit the Changelog entry. |
万康 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
@MaxWk Why it is in draft stage? The change should be quite easy and we need this feature... |
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.
Everything looks alright :)
@Mergifyio update |
✅ Branch has been successfully updated |
@Mergifyio update |
✅ Branch has been successfully updated |
Sorry for late reply, This pull request was forgotten by me, I thought it had been merged |
Still not merged - the test about this feature has failed. |
Any news here? |
Update |
really need |
update |
@alexey-milovidov @abyss7 I'm sorry it took so long to do this, I've been so busy with work lately, please help me review |
@Mergifyio update |
✅ Branch has been successfully updated |
@qoega These errors seems not relate to my update |
Yes. Flaky check just shows that timeout was hit before the number of ribs or wanted to run. Absolutely normal if test is long. |
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 code LGTM.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added a setting
exact_rows_before_limit
(0/1). When enabled, ClickHouse will provide exact value forrows_before_limit_at_least
statistic, but with the cost that the data before limit will have to be read completely.This closes #6613.
Detailed description / Documentation draft:
A setting exact_rows_before_limit = 0|1. When enabled, LimitBlockInputStream will always_read_till_end (we already have this feature in code) and the field rows_before_limit_at_least will have precise value.
#6613
By adding documentation, you'll allow users to try your new feature immediately, not when someone else will have time to document it later. Documentation is necessary for all features that affect user experience in any way. You can add brief documentation draft above, or add documentation right into your patch as Markdown files in docs folder.
If you are doing this for the first time, it's recommended to read the lightweight Contributing to ClickHouse Documentation guide first.
Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/