-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](cache stat) add builder stat to scanner (#56165) #59515
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
base: branch-3.1
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Pull request overview
This PR adds builder statistics tracking to the scanner by modifying the get_segment_num_rows method to accept and populate an OlapReaderStatistics parameter. This enables collection of file cache statistics during segment metadata loading.
Key Changes:
- Added
get_segment_num_rowsmethod toBetaRowsetclass with statistics parameter support - Modified
ParallelScannerBuilderto track and expose builder statistics viabuilder_stats()accessor - Updated
OlapScanOperatorto report file cache statistics from the scanner builder to metrics and profiles
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| be/src/olap/rowset/beta_rowset.h | Added get_segment_num_rows method declaration and member variables for caching segment rows using thread-safe DorisCallOnce pattern |
| be/src/olap/rowset/beta_rowset.cpp | Implemented get_segment_num_rows method with lazy initialization of segment row counts and statistics tracking |
| be/src/olap/rowset/beta_rowset_reader.cpp | Refactored to call BetaRowset::get_segment_num_rows instead of accessing internal state directly |
| be/src/olap/parallel_scanner_builder.h | Added _builder_stats member and builder_stats() accessor method |
| be/src/olap/parallel_scanner_builder.cpp | Updated to use new get_segment_num_rows API and pass statistics parameter |
| be/src/pipeline/exec/olap_scan_operator.cpp | Added file cache profile reporting and metrics updates from builder statistics |
| be/test/olap/segcompaction_test.cpp | Updated test calls to use new BetaRowset::get_segment_num_rows API with statistics parameter |
| be/test/olap/segcompaction_mow_test.cpp | Updated test calls to use new BetaRowset::get_segment_num_rows API with statistics parameter |
| be/test/olap/rowid_conversion_test.cpp | Updated test calls to use new BetaRowset::get_segment_num_rows API with statistics parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Rows read from storage. | ||
| // Include the rows read from doris page cache. | ||
| _scan_rows = ADD_COUNTER(_runtime_profile, "ScanRows", TUnit::UNIT); | ||
| _scan_rows = ADD_COUNTER(custom_profile(), "ScanRows", TUnit::UNIT); |
Copilot
AI
Dec 31, 2025
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 method call custom_profile() appears to be undefined. Looking at the surrounding code in this file, profile counters are typically added to _runtime_profile, _scanner_profile, or _segment_profile. The method custom_profile() is not defined in the base classes (PipelineXLocalStateBase, ScanLocalState, etc.) and this will likely cause a compilation error. This should probably be _runtime_profile instead to match the pattern used in line 69 and elsewhere in the codebase.
| _scan_rows = ADD_COUNTER(custom_profile(), "ScanRows", TUnit::UNIT); | |
| _scan_rows = ADD_COUNTER(_runtime_profile, "ScanRows", TUnit::UNIT); |
3c0c124 to
44e535c
Compare
|
run buildall |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)