You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "BiteTheDDDDt (via GitHub)" <gi...@apache.org> on 2026/03/23 06:50:15 UTC
[PR] [Improvement](scan) Update scanner limit controller [doris]
BiteTheDDDDt opened a new pull request, #61617:
URL: https://github.com/apache/doris/pull/61617
This pull request introduces a shared, atomic row limit mechanism in the scanner context to ensure that concurrent scanners collectively respect the SQL `LIMIT` clause. The main changes implement a thread-safe, centrally managed quota for remaining rows, preventing over-scanning and efficiently coordinating concurrent scanner threads. Additionally, related logic is updated to stop or throttle scanners when the quota is exhausted and to provide improved debug information.
**Shared limit management and enforcement:**
* Added a new atomic member `_remaining_limit` to `ScannerContext`, representing the shared remaining row limit across all scanners, and initialized it appropriately. Provided an `acquire_limit_quota()` method for atomically claiming rows from this quota. [[1]](diffhunk://#diff-0c9a817d45d8130ea3211189e1321d1275e22fd4a9a3fac2bd707b1cfeefa5e5R74) [[2]](diffhunk://#diff-0c9a817d45d8130ea3211189e1321d1275e22fd4a9a3fac2bd707b1cfeefa5e5R99) [[3]](diffhunk://#diff-3049f42cade971254aae07ced700d9a10b2505b03da743efea3270e63bd88dceR224-R229)
* Updated scanner scheduling logic to check and respect the shared limit before launching new scan tasks or continuing scanning, ensuring no new work is scheduled if the limit is exhausted. [[1]](diffhunk://#diff-0c9a817d45d8130ea3211189e1321d1275e22fd4a9a3fac2bd707b1cfeefa5e5R639-R643) [[2]](diffhunk://#diff-ecdf52f3fb33b9018cc1aff92085e470087071b25d79efed8a849a289215d05fR235-R239)
**Block quota enforcement and block truncation:**
* In the scanner execution loop, after reading a block, scanners now atomically acquire quota for the number of rows in the block. If quota is exhausted, the block is discarded or truncated to the permitted row count, and scanning stops.
**Completion and lifecycle management:**
* Modified context completion logic to also mark the scan as finished if the shared limit is exhausted and no scanners are running, ensuring correct query termination.
**Debugging and observability:**
* Enhanced the `debug_string()` output of `ScannerContext` to include the current value of `remaining_limit`, aiding in diagnostics and monitoring.
**Small-limit optimization:**
* Retained and clarified the optimization for scanners with a small per-scanner limit, ensuring they return early to avoid unnecessary data scanning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4108404857
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "zclllyybb (via GitHub)" <gi...@apache.org>.
zclllyybb commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4111656859
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt merged PR #61617:
URL: https://github.com/apache/doris/pull/61617
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4116382607
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4116694787
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4116696116
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4116449759
Maybe we should maintain this counter in ScanOperator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #61617:
URL: https://github.com/apache/doris/pull/61617#discussion_r2980249873
##########
be/src/exec/scan/scanner_context.h:
##########
@@ -115,7 +115,7 @@ class ScannerContext : public std::enable_shared_from_this<ScannerContext>,
const TupleDescriptor* output_tuple_desc,
const RowDescriptor* output_row_descriptor,
const std::list<std::shared_ptr<ScannerDelegate>>& scanners, int64_t limit_,
- std::shared_ptr<Dependency> dependency
+ std::shared_ptr<Dependency> dependency, std::atomic<int64_t>* shared_scan_limit
#ifdef BE_TEST
Review Comment:
**[Critical/Build-Breaking]** This constructor signature change adds `shared_scan_limit` as a new required parameter between `dependency` and the `BE_TEST`-only `num_parallel_instances`. However, **all 14 callsites in `be/test/exec/scan/scanner_context_test.cpp` were not updated**.
In `BE_TEST` mode, the constructor now expects 9 arguments:
```
(state, local_state, output_tuple_desc, output_row_descriptor, scanners, limit_, dependency, shared_scan_limit, num_parallel_instances)
```
But the tests still pass only 8 arguments, with the 8th being an `int` (`parallel_tasks`) that will be implicitly matched to the `std::atomic<int64_t>*` parameter — which will either fail to compile or cause undefined behavior.
The test file needs:
1. A `std::atomic<int64_t>` member (e.g., `std::atomic<int64_t> shared_limit {-1};`) in the test fixture.
2. All `create_shared` calls updated to pass `&shared_limit` before `parallel_tasks`.
##########
be/src/exec/scan/scanner_context.cpp:
##########
@@ -56,7 +56,8 @@ ScannerContext::ScannerContext(RuntimeState* state, ScanLocalStateBase* local_st
const TupleDescriptor* output_tuple_desc,
const RowDescriptor* output_row_descriptor,
const std::list<std::shared_ptr<ScannerDelegate>>& scanners,
- int64_t limit_, std::shared_ptr<Dependency> dependency
+ int64_t limit_, std::shared_ptr<Dependency> dependency,
+ std::atomic<int64_t>* shared_scan_limit
#ifdef BE_TEST
,
int num_parallel_instances
Review Comment:
**[Medium/Liveness concern]** Consider this scenario:
1. There are 10 scanners total, max concurrency = 2. Scanner A and Scanner B are scheduled (`_num_scheduled_scanners = 2`).
2. Scanner A produces a block, calls `acquire_limit_quota()`, gets all remaining quota → `_shared_scan_limit` becomes 0. Scanner A is marked `eos`.
3. Scanner B hasn't produced any blocks yet. It checks `remaining_limit() == 0` at the top of the while loop (scanner_scheduler.cpp:236) → marks `eos`.
4. Both scanners push back their tasks via `push_back_scan_task` → `_num_scheduled_scanners` goes to 0, `_dependency->set_ready()`.
5. `get_block_from_queue` is called. It processes Scanner A's task (increments `_num_finished_scanners`). Then it tries to `schedule_scan_task(nullptr, l)`. Inside, `_pull_next_scan_task(nullptr, ...)` checks `_shared_scan_limit->load() == 0` → returns `nullptr`. No new scanners scheduled.
6. Processing Scanner B's task similarly.
7. Eventually, `_tasks_queue` is empty, `_shared_scan_limit == 0`, `_num_scheduled_scanners == 0` → finish condition fires. **This works correctly.**
However, there's a subtlety: between step 4 (scanner A pushes back) and step 4 (scanner B pushes back), `get_block_from_queue` may run and find `_tasks_queue` not empty (A's task) but after consuming A's block, it finds `_tasks_queue` is now empty AND `_num_scheduled_scanners == 1` (B still running). The finish condition fails. Then `_dependency->block()` fires. This is fine because when B eventually pushes back, it calls `_dependency->set_ready()`, which re-triggers `get_block_from_queue`.
**The logic appears correct**, but this should have unit tests covering the shared limit exhaustion path to confirm.
##########
be/src/exec/scan/scanner_scheduler.cpp:
##########
@@ -232,6 +232,11 @@ void ScannerScheduler::_scanner_scan(std::shared_ptr<ScannerContext> ctx,
eos = true;
break;
}
+ // If shared limit quota is exhausted, stop scanning.
+ if (ctx->remaining_limit() == 0) {
+ eos = true;
+ break;
Review Comment:
**[Nit/Performance]** This `remaining_limit() == 0` check at the top of the while loop is redundant with the `acquire_limit_quota` check below (lines 280-291) for all iterations after the first. After the first block is produced, if `acquire_limit_quota` returns 0, the loop already breaks. This top-of-loop check is only useful if a *different* scanner exhausted the limit between iterations.
This is fine for correctness and is a reasonable optimization to avoid the overhead of `get_block` when the limit is already exhausted. Just noting that it is not strictly necessary — the `acquire_limit_quota(block_rows)` at line 281 would catch it anyway. Keep it for the early-exit benefit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4108415102
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4116382798
PR approved by anyone and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4117243695
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4118131577
# BE Regression && UT Coverage Report
Increment line coverage `100% (0/0)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/61617_19e8fb70919e7888fbf186bf6778da55b8de229f_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/61617_19e8fb70919e7888fbf186bf6778da55b8de229f_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 71.61% (26377/36834) |
| Line Coverage | 54.45% (277546/509681) |
| Region Coverage | 51.73% (230493/445550) |
| Branch Coverage | 53.11% (99269/186920) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4123007336
# BE Regression && UT Coverage Report
Increment line coverage `100% (0/0)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/61617_19e8fb70919e7888fbf186bf6778da55b8de229f_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/61617_19e8fb70919e7888fbf186bf6778da55b8de229f_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 71.61% (26377/36834) |
| Line Coverage | 54.46% (277548/509681) |
| Region Coverage | 51.72% (230457/445550) |
| Branch Coverage | 53.11% (99269/186920) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #61617:
URL: https://github.com/apache/doris/pull/61617#issuecomment-4124111482
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Improvement](scan) Update scanner limit controller [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #61617:
URL: https://github.com/apache/doris/pull/61617#discussion_r2976012921
##########
be/src/exec/scan/scanner_context.cpp:
##########
@@ -95,6 +96,7 @@ ScannerContext::ScannerContext(RuntimeState* state, ScanLocalStateBase* local_st
};
if (limit < 0) {
limit = -1;
+ _remaining_limit.store(-1, std::memory_order_relaxed);
Review Comment:
**[Medium] Missing `limit == 0` handling.** The constructor handles `limit < 0` (normalizing to -1) but does not handle `limit == 0`. If `limit_=0` were ever passed:
- `_remaining_limit` starts at 0
- `_pull_next_scan_task` refuses to schedule any scanner (remaining == 0 check)
- No scanner runs, so `push_back_scan_task` is never called, `_dependency->set_ready()` is never invoked
- The scan operator is permanently blocked → query hangs
While the FE's `EliminateLimit` rule currently prevents `LIMIT 0` from reaching the scan layer, the BE code should be self-consistent. Consider adding a guard in `init()` similar to the existing empty-scanners check:
```cpp
if (limit == 0) {
_is_finished = true;
_set_scanner_done();
return Status::OK();
}
```
Or at minimum handle it in the constructor alongside the `limit < 0` case.
##########
be/src/exec/scan/scanner_context.h:
##########
@@ -221,6 +221,12 @@ class ScannerContext : public std::enable_shared_from_this<ScannerContext>,
int _batch_size;
// The limit from SQL's limit clause
int64_t limit;
+ // Shared remaining limit across all concurrent scanners, decremented atomically.
+ // -1 means no limit. Scanners call acquire_limit_quota() to claim rows.
+ std::atomic<int64_t> _remaining_limit;
+ // Atomically acquire up to `desired` rows. Returns actual granted count (0 = exhausted).
+ int64_t acquire_limit_quota(int64_t desired);
Review Comment:
**[Nit] Access specifier placement.** `acquire_limit_quota()` and `remaining_limit()` are declared in the `protected` section but called from `ScannerScheduler::_scanner_scan()` (a friend class) and the scanner scan loop. Since `ScannerScheduler` is already a `friend`, this works, but consider whether these should be `public` for clarity — especially `remaining_limit()` which is a simple accessor with no side effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org