-
Notifications
You must be signed in to change notification settings - Fork 409
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 new items in scan context #7841
Add new items in scan context #7841
Conversation
…yunyan_scan_context_s3
And there are some ColumnFile in the DeltaValueSpace read from local cache or fetch from remote write nodes. We should add them to tiflash/dbms/src/Storages/DeltaMerge/Remote/RNWorkerFetchPages.cpp Lines 41 to 71 in 96fbffd
|
…yunyan_scan_context_s3
const auto check_results = filter->roughCheck(0, pack_count, param); | ||
std::transform(use_packs.begin(), use_packs.end(), check_results.begin(), use_packs.begin(), [](UInt8 a, RSResult b) { return (static_cast<bool>(a)) && (b != None); }); | ||
scan_context->total_dmfile_rough_set_index_load_time_ns += watch.elapsed(); |
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.
rename to total_dmfile_rough_set_index_check_time_ns
?
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, we can distinguish the loading time
and checking time
of rough set index
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.
Does this mean that we need to use two indicators separately for statistics?
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 the sum is enough.
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 rest LGTM
/run-all-tests |
/run-all-tests |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/run-all-tests |
…unyan/tiflash into hongyunyan_scan_context_s3
/run-all-tests |
@hongyunyan: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests
If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: ref #5926
Problem Summary:
What is changed and how it works?
Add more information in Scan Context:
Check List
Tests
Side effects
Documentation
Release note