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
Improve CHECK TABLE system query #52745
Conversation
a4b2a73
to
d515b6f
Compare
This is an automated comment for commit 1183dac with description of existing statuses. It's updated for the latest CI running
|
I'm not sure, but I thought this query is single-threaded.. Is it true? Can we make it parallelized? |
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.
I think we have tests for progress, maybe it make sense to add it here.
|
||
check_results.push_back(check_result); | ||
|
||
bool should_continue = check_result.success || !check_query_single_value_result; |
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.
Isn't it a semantic change? I think we should process all data parts regardless of check_query_single_value_result
.
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.
I assumed it was good to have a mode with early exit and thought we could do it since the user couldn't know how many errors there were with single-value result. However, it's doubtful, so I returned it to the original behavior.
75948b2
to
5f14590
Compare
I updated the implementation to build a more sophisticated pipeline and use its features properly. It allowed to eliminate I split I also added a doc (we have this query documented, so I added more information and examples) and added a test for progress. |
5f14590
to
de315d3
Compare
de315d3
to
bb22394
Compare
|
||
Chunk generate() override | ||
{ | ||
if (is_valuer_emitted.exchange(true)) |
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.
we return chunk only once, right?
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.
Yes, only one block with single row is expected in the result
bb22394
to
ceaee43
Compare
The path $ cd docs/en/sql-reference/statements
$ cat ../../operations/settings/settings.md | grep -F settings-max_threads
## max_threads {#settings-max_threads}
Parallel `INSERT SELECT` has effect only if the `SELECT` part is executed in parallel, see [max_threads](#settings-max_threads) setting.
$ |
ceaee43
to
dc98d33
Compare
dc98d33
to
1183dac
Compare
And the CI caught it, it failed on commit dc98d33: |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
CHECK TABLE
has better performance and usability (sends progress updates, cancellable)TODO