[branch-4.1][fix](load) only load source scanners update load counters#65276
Open
liaoxin01 wants to merge 1 commit into
Open
[branch-4.1][fix](load) only load source scanners update load counters#65276liaoxin01 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
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect load-counter accounting in BE scans by ensuring only scanners that actually read load source data contribute “filtered/unselected” rows to RuntimeState load counters. This prevents negative num_rows_load_success() values (and downstream insert failures) when rows are filtered by query predicates during INSERT ... SELECT, DELETE ... WHERE EXISTS, or UPDATE ... IN (subquery).
Changes:
- Gate
Scanner::_collect_profile_before_close()updates to load counters behind a new virtual_should_update_load_counters()hook. - Override the hook in
FileScannerto also treatTFileType::FILE_STREAMscans (TVF-based load sources) as load-counter contributors. - Add a regression test + expected output to validate no-op insert/delete/update paths don’t fail when the source scan filters out all rows (including when
enable_profile=true).
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 | Adds regression coverage ensuring scan-predicate filtering doesn’t “pollute” load counters and cause negative loaded-row calculations. |
| regression-test/data/load_p0/insert/test_scan_filtered_rows_not_pollute_load_counter.out | Expected results for the new regression suite. |
| be/src/exec/scan/scanner.h | Introduces _should_update_load_counters() to control whether a scanner reports filtered/unselected rows into load counters. |
| be/src/exec/scan/scanner.cpp | Gates load-counter updates in _collect_profile_before_close() using _should_update_load_counters(). |
| be/src/exec/scan/file_scanner.h | Declares FileScanner override of _should_update_load_counters(). |
| be/src/exec/scan/file_scanner.cpp | Implements override to include FILE_STREAM TVF-based load source scans even when _is_load is false. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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