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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
