[branch-4.0][fix](load) only load source scanners update load counters#65277
Open
liaoxin01 wants to merge 1 commit into
Open
[branch-4.0][fix](load) only load source scanners update load counters#65277liaoxin01 wants to merge 1 commit into
liaoxin01 wants to merge 1 commit into
Conversation
) - Gate the load counter update in `Scanner::_collect_profile_before_close()` on a new virtual `_should_update_load_counters()`: - Base `Scanner` reports only when `_is_load` (classic stream/broker/routine load scanners with src tuple desc). - `FileScanner` additionally reports for `FILE_STREAM` scans: TVF based loads (`http_stream`, group commit) plan the load source as a tvf query scan without src tuple desc (`_is_load` is false), but their WHERE clause filtered rows must still be reported as `NumberUnselectedRows` / counted into `NumberTotalRows`. - Add a deterministic regression case covering INSERT-SELECT / DELETE / UPDATE whose scans filter out all rows. For DELETE/UPDATE/INSERT INTO ... SELECT executed through the insert path, rows filtered by query scan predicates (including runtime filters) were added to the RuntimeState load counters. When all scanned rows are filtered, `num_rows_load_success()` (total - filtered - unselected) goes negative, BE reports a negative `dpp.norm.ALL`, and FE fails the `insert_max_filter_ratio` check with errors like: ``` Insert has too many filtered data 0/-2 insert_max_filter_ratio is 1.000000 ``` This only triggers with `enable_insert_strict=false` and `insert_max_filter_ratio > 0` (the strict branch only checks `filteredRows > 0`). The intermittency in the field comes from runtime filter arrival timing: rows are only counted when the RF arrives in time to filter inside the scanner. This was historically gated by `if (!enable_profile && !_is_load) return;` (already buggy with `enable_profile=true`), and became unconditional after apache#57314 removed the early return. Compared to the previous revision of this PR (thrift `skip_query_scan_load_counters` option set by FE for DELETE/UPDATE): - No thrift / FE changes needed. - Also fixes plain INSERT INTO ... SELECT: `AbstractInsertExecutor` sets `query_type=LOAD` for all insert-path commands, so the query-type based gate still let OlapScanner predicate-filtered rows pollute the counters there. - `FILE_STREAM` is a precise discriminator: only the `http_stream` / `group_commit` TVFs use it, and they require a backend id on the ConnectContext (only present for stream-load style HTTP requests), so they can never appear in a normal query/DELETE/UPDATE. `FileScanner::_counter.num_rows_filtered` is only accumulated in `_convert_to_output_block` (load-only path), so for `http_stream` the `NumberFilteredRows` reported to clients comes from sink validation and is unaffected by this gate. - New regression case `regression-test/suites/load_p0/insert/test_scan_filtered_rows_not_pollute_load_counter.groovy`: uses value-column predicates on an AGGREGATE KEY table (cannot be pushed down to storage, evaluated by scanner conjuncts) to deterministically reproduce; before this fix the INSERT-SELECT/DELETE/UPDATE statements fail with `0/-10`. - `test_group_commit_http_stream` semantics preserved: `insert into ... select from http_stream(...) where ...` still reports `NumberUnselectedRows`/`NumberTotalRows` (the previous `_is_load`-only attempt broke this with `expected: <6> but was: <5>`).
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect load counter accounting by ensuring only scanners that are actually reading load source data update RuntimeState load counters. This prevents query predicate filtering (e.g., INSERT INTO ... SELECT ... WHERE ...) from being mixed into load counters, which could otherwise make num_rows_load_success() negative and trigger erroneous “too many filtered rows” failures.
Changes:
- Gate
Scanner::_collect_profile_before_close()load-counter updates behind a new virtual_should_update_load_counters()hook. - Override
_should_update_load_counters()inFileScannerto also treatFILE_STREAMscans as load-source scans (TVF-based load paths where_is_loadis false). - Add a regression test covering INSERT/DELETE/UPDATE no-op cases where all rows are filtered during scan, ensuring the load counter logic does not cause failures (including with profile enabled).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/load_p0/insert/test_scan_filtered_rows_not_pollute_load_counter.groovy | New regression suite validating scan-filtered rows don’t corrupt load counters for INSERT/DELETE/UPDATE paths. |
| regression-test/data/load_p0/insert/test_scan_filtered_rows_not_pollute_load_counter.out | Expected outputs for the new regression suite. |
| be/src/vec/exec/scan/scanner.h | Introduces _should_update_load_counters() to control whether a scanner contributes to load counters. |
| be/src/vec/exec/scan/scanner.cpp | Gates load counter updates in _collect_profile_before_close() using _should_update_load_counters(). |
| be/src/vec/exec/scan/file_scanner.h | Declares FileScanner override of _should_update_load_counters(). |
| be/src/vec/exec/scan/file_scanner.cpp | Implements override to update load counters for _is_load scanners and FILE_STREAM TVF load scanners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
run buildall |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pick #63781