[fix](be) Harden FileScannerV2 schema and reader edge cases#65548
[fix](be) Harden FileScannerV2 schema and reader edge cases#65548Gabriel39 wants to merge 13 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 20th, 2026 12:23 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 30262 ms |
TPC-DS: Total hot run time: 180840 ms |
ClickBench: Total hot run time: 24.99 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 20th, 2026 12:23 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found four issues that should be fixed before merging.
Checkpoint summary: the PR is focused on FileScannerV2/format-v2 edge cases and has useful unit/regression coverage, but the current implementation still has correctness issues around stop-vs-empty EOF handling and unsupported Parquet logical types. The page-cache range refactor also introduces two performance/lifecycle regressions: non-exact warm-cache reuse is lost across readers, and the new reader-local index is unbounded. I did not run BE/regression tests in this review-only runner; the checkout is a single shallow commit with the base/parent SHA unavailable, so validation was static against the bundled authoritative PR diff and GitHub PR metadata. User focus: no additional focus points were provided.
|
/review |
|
run buildall |
There was a problem hiding this comment.
I found two remaining correctness issues after the current fixes: primitive COUNT(column) aggregate pushdown can still bypass the unsupported Parquet logical-type validation, and the generic EOF skip can still downgrade a malformed Native file into an empty split. I rechecked the scanner EOF, unsupported Parquet type, nested column-mapper, and page-cache range-directory surfaces against the existing review threads and did not find additional distinct findings.
Validation was static only; I did not run builds or tests because this review invocation is read-only per the bundled prompt.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29659 ms |
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found one remaining correctness issue in the COUNT aggregate request construction. The rest of the reviewed areas look covered by the current fixes or by existing review threads.
Checkpoint conclusions: the PR goal is clear and mostly addressed; the change is focused around FileScannerV2/format-v2 review fixes; I did not find new concurrency or lifecycle problems in the scanner stop/empty split or page-cache range-directory paths; no new config or persistence compatibility changes are introduced; existing parallel paths were checked for scanner EOF, Parquet/ORC aggregate semantics, and page-cache behavior; test coverage was reviewed statically but not run in this review-only environment; observability changes are limited to the empty-file counter and look consistent. User focus: no additional user-provided focus beyond the full PR review.
TPC-H: Total hot run time: 29893 ms |
TPC-DS: Total hot run time: 178193 ms |
ClickBench: Total hot run time: 25.01 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: File aggregate pushdown inferred COUNT(column) from a single post-pruning scan mapping. COUNT(*) also retains an arbitrary placeholder slot, so a nullable placeholder made Parquet and ORC readers count non-null values instead of rows. Carry the semantic COUNT arguments from Nereids through the scan thrift contract, keep true COUNT(*) column-free, and fall back when an explicit COUNT argument cannot be mapped directly. Also default split-level conjunct overrides to nullopt so existing standalone readers compile and preserve their initial predicate snapshot. ### Release note Fix COUNT(*) results for Parquet and ORC file scans when the retained placeholder column contains NULL values. ### Check List (For Author) - Test: Unit Test - 7 targeted Backend ASAN unit tests - Added Frontend rule coverage for COUNT(*) and COUNT(column); execution was blocked by unrelated untracked cloud storage sources missing vendor SDKs in the validation workspace - Behavior changed: Yes (file aggregate pushdown now distinguishes COUNT(*) from COUNT(column) explicitly) - Does this need documentation: No
Gabriel39
requested review from
924060929,
englefly and
morrySnow
as code owners
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: File-local nested predicate rewriting ignored nullable-file to required-table schema contracts, allowing NULL rows to be filtered before TableReader could reject them. Iceberg equality-delete also used an arbitrary physical column as a row-count carrier when all keys were missing, so an unsupported unprojected Parquet leaf could fail an otherwise valid projection. Keep such nested predicates above TableReader and use virtual row position as the equality-delete carrier. ### Release note Fix schema-evolved nested predicate validation and Iceberg equality-delete scans with unsupported unprojected Parquet columns. ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="ColumnMapperCastTest.NestedElementAt*:IcebergV2ReaderTest.IcebergEqualityDelete*" -j 96 (19 tests passed) - Behavior changed: Yes. Unsafe nested predicates remain table-level, and missing-key equality deletes use virtual row position for batch sizing. - Does this need documentation: No
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
run buildall |
There was a problem hiding this comment.
I found one remaining correctness issue in the current head after checking the updated COUNT argument plumbing and existing review threads.
Checkpoint conclusions:
- Goal/current code: the PR is a focused follow-up for FileScannerV2/format-v2 EOF handling, Parquet unsupported type validation, page-cache range metadata, struct predicate localization, and explicit COUNT argument propagation. Most previously reported paths are addressed, but external table-level COUNT split planning still has a wrong-result path for COUNT(col).
- Scope: the changes are concentrated and supported by BE/FE unit tests plus a TVF regression case.
- Concurrency/lifecycle: scanner stop-vs-empty EOF handling, TableReader abort/close paths, and page-cache range-directory locking/caps were reviewed; no new non-duplicate lifecycle issue was found.
- Compatibility/parallel paths: FE translator, V1 scanner, V2 scanner/TableReader, table-level row count, Parquet aggregate validation, Iceberg/Paimon table-format planning, and rolling-upgrade duplicate context were checked. The accepted issue is distinct from the existing BE table-level-row-count comment because it happens before BE fallback.
- Tests: reviewed statically. I did not run builds or tests because the review prompt makes this a read-only review runner, and this checkout is missing
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - User focus: no additional user-provided review focus.
Subagent convergence: Round 1 used FE, BE, format-v2, COUNT-risk, and EOF/unsupported/page-cache-risk subagents. Round 2 reran convergence after accepting the COUNT split-pruning finding; both convergence agents returned NO_NEW_VALUABLE_FINDINGS.
| @Override | ||
| protected void toThrift(TPlanNode planNode) { | ||
| planNode.setPushDownAggTypeOpt(pushDownAggNoGroupingOp); | ||
| planNode.setPushDownCountSlotIds( |
There was a problem hiding this comment.
This still leaves a pre-BE table-format COUNT path using the raw COUNT flag. For COUNT(required_col), the planner now serializes a non-empty push_down_count_slot_ids, and BE correctly falls back instead of consuming table-level row counts. But Iceberg and Paimon decide their table-level count split planning before BE sees this field: IcebergScanNode.isBatchMode()/doGetSplits() and PaimonScanNode check only getPushDownAggNoGroupingOp() == COUNT, then keep just one or parallel * numBackends count splits and assign the total row count to them. Since BE ignores that assigned count for non-empty arguments, it scans only the retained subset of real splits, so COUNT(non_null_col) can undercount any table with more data splits than the retained representatives. Please gate the Iceberg/Paimon table-level count split reduction on the explicit COUNT(*) marker, or otherwise avoid dropping real splits whenever pushDownCountSlotIds is non-empty.
TPC-H: Total hot run time: 29339 ms |
TPC-DS: Total hot run time: 178329 ms |
ClickBench: Total hot run time: 24.99 s |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: Iceberg and Paimon table-level COUNT planning used only the aggregate opcode, so COUNT(column) could discard most real data splits and attach a metadata row count. Backend correctly rejects that shortcut for explicit COUNT arguments and falls back to reading the column, but then scans only the retained representatives and undercounts multi-split tables. Restrict FE metadata COUNT split reduction to the explicit COUNT(*) marker and preserve all real splits for COUNT(column). ### Release note Fix incorrect COUNT(column) results on multi-split Iceberg and Paimon tables when table-level count pushdown is enabled. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.datasource.iceberg.source.IcebergScanNodeTest,org.apache.doris.datasource.paimon.source.PaimonScanNodeTest (23 tests passed) - Behavior changed: Yes. Only COUNT(*)/COUNT(1) may replace real Iceberg or Paimon splits with metadata count representatives; COUNT(column) keeps all real splits. - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review complete. I found one remaining correctness issue and submitted it inline.
Checkpoint conclusions:
- Goal/test coverage: the PR mostly addresses the COUNT argument split, empty-file EOF handling, unsupported Parquet validation, page-cache range reuse, and struct-predicate localization, but it is missing coverage for
COUNT(*)when column pruning keeps an unsupported Parquet placeholder. - Scope/focus: the changes are focused on the reviewed scanner/format/planner paths. The user focus file contains no additional focus points.
- Compatibility and parallel paths: reviewed FE/BE thrift propagation, V1/V2 scanners, TableReader, Iceberg/Paimon table-level count handling, and Hive/TVF parallel paths. No additional non-duplicate issue found.
- Error handling and data correctness: reviewed EOF status propagation and unsupported logical-type rejection. Native zero-byte EOF concerns are already covered by existing review threads; the inline comment covers the remaining distinct data-correctness gap.
- Concurrency/lifecycle/performance: reviewed page-cache range directory lifetime, locking, stale-range pruning, and split/reader lifecycle. No additional issue found.
- Tests: static review only; no builds or tests were run per the review prompt.
Round 2 convergence completed with all subagents returning NO_NEW_VALUABLE_FINDINGS for the final comment set.
| // bloom filters or page indexes inspect their physical fallback type. For example, a predicate | ||
| // on TIME_MILLIS must fail here even when its INT32 statistics would prune every row group; | ||
| // otherwise the same unsupported SELECT could fail or silently succeed depending on data. | ||
| RETURN_IF_ERROR(validate_requested_columns_supported(_state->file_schema, *request_snapshot)); |
There was a problem hiding this comment.
This still lets an explicit COUNT(*) observe the column-pruning placeholder before the aggregate path can use the empty COUNT request. TableReader::open_reader() builds a normal FileScanRequest from _projected_columns and calls reader->open(file_request) before _try_materialize_aggregate_pushdown_rows(). Since Nereids keeps one minimum-width scan slot when COUNT(*) needs no output slots, a file whose retained placeholder is an unsupported TIME_MILLIS leaf reaches this validation in non_predicate_columns and fails, even though the later COUNT() aggregate request would correctly have no columns and could answer from row-group counts. Please skip placeholder validation/open for explicit COUNT() metadata pushdown, or use a supported/virtual carrier, and add coverage where COUNT(*) is planned over an unsupported placeholder column.
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: Explicit COUNT(*) keeps a planner placeholder scan slot even though file aggregate metadata needs no column. TableReader opened the normal Parquet scan request before aggregate materialization, so an unsupported logical leaf such as adjusted TIME_MILLIS could be rejected as if it were user-projected and make COUNT(*) fail. Mark non-predicate columns as placeholders only after the existing COUNT(*) aggregate safety gate succeeds, and skip unsupported-type validation only for those placeholders while preserving strict predicate, COUNT(column), and ordinary projection validation. ### Release note Fix COUNT(*) failures on Parquet files where column pruning retains an unsupported logical-type placeholder. ### Check List (For Author) - Test: Unit Test - run-be-ut.sh targeted 4 tests (all passed) - Behavior changed: Yes. Eligible COUNT(*) metadata pushdown ignores planner-only non-predicate placeholder validation; actual projections and COUNT(column) remain unchanged. - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
I completed the requested review cycle against the authoritative diff and existing review threads, including a second convergence round after accepting the remaining candidate. Subagents D, E, and the focused M1 reviewer all returned NO_NEW_VALUABLE_FINDINGS for the current final comment set.
I found one remaining non-duplicate correctness issue: explicit COUNT() can still fail in the delete-forced row-scan fallback path because the arbitrary retained placeholder is only marked as a COUNT() placeholder when aggregate metadata pushdown is supported. Existing threads already cover the rolling-upgrade, V1/table-level COUNT, Native EOF, page-cache, struct nullability, equality-delete carrier, and metadata-eligible COUNT(*) placeholder cases, so I did not duplicate them.
Review checkpoints: the patch is focused on prior review fixes, but M1 leaves a correctness gap; FE/BE COUNT propagation, V1/V2 scanner behavior, Iceberg/Paimon table-level split planning, parquet unsupported-column validation, Native EOF handling, struct predicate localization, page-cache lifecycle, and delete/equality-delete carriers were checked. No config, transaction, persistence, write-path, or observability change introduced a separate finding in this diff. I did not run builds or tests, per the review-only instructions.
| // would fail before the empty aggregate request can count rows from footer metadata. Mark | ||
| // placeholders only after the same safety gate used by aggregate materialization succeeds; | ||
| // COUNT(col), filters, deletes and pending runtime filters keep normal column validation. | ||
| file_request->non_predicate_columns_are_count_star_placeholders = |
There was a problem hiding this comment.
This still leaves a COUNT() fallback path observing the arbitrary placeholder column. The marker is set only when _supports_aggregate_pushdown(COUNT) is true, but that returns false as soon as position deletes, deletion vectors, or Iceberg equality deletes are present. When table-level row count is not assigned, get_block() then falls through to the normal file-reader path; Parquet validates and later reads every non-predicate column because the marker is false, while delete filtering only needs the virtual row-position predicate column. So an Iceberg split whose retained COUNT() placeholder is an unsupported TIME_MILLIS leaf can reject COUNT(*) even though the value is never part of the aggregate. Please carry the explicit COUNT(*) placeholder state into the delete-filter fallback path and use a row-position/default-row carrier, or otherwise avoid validating/reading the placeholder while still validating real predicate/delete columns.
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: Explicit COUNT(*) keeps a planner-only scan slot after column pruning. The previous implementation marked that slot only when metadata aggregate pushdown was eligible. Position deletes, deletion vectors, equality deletes, predicates, or pending runtime filters disable aggregate pushdown and fall back to row scanning, causing Parquet to validate and decode the arbitrary placeholder again. An unsupported logical leaf such as adjusted TIME_MILLIS could therefore reject COUNT(*) even though only the surviving row count was needed. Record exact COUNT(*) placeholder column ids independently of aggregate eligibility, keep real predicate and delete columns strict, and synthesize default values for surviving placeholder rows without physical column IO. ### Release note Fix COUNT(*) fallback scans over Parquet files whose planner-only placeholder uses an unsupported logical type. ### Check List (For Author) - Test: Unit Test - 4 targeted BE ASAN unit tests passed - Behavior changed: Yes. COUNT(*) fallback scans no longer validate or read planner-only non-predicate placeholders; predicates, delete keys, COUNT(column), and normal projections remain unchanged. - Does this need documentation: No
|
/review |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
I found two issues that should be fixed before this lands.
- A struct-child predicate fallback can run a non-lossless file-to-table cast below TableReader and filter away rows that normal table materialization would reject.
- Hive and TVF COUNT(col) fallback scans can still lose normal file-split parallelism because their no-split shortcut checks the raw COUNT opcode instead of the semantic COUNT(*) state.
Review checkpoints: the PR addresses several prior review issues, but these two gaps remain. Compatibility and mixed-version COUNT handling were reviewed; the new-field rolling-upgrade concern is already covered by an existing thread. BE scanner/format lifecycle, EOF handling, unsupported Parquet placeholder handling, page-cache range metadata, FE COUNT argument propagation, and changed test coverage were rechecked with subagents. No additional user-provided review focus was specified. I did not run builds or tests in this review-only runner.
| if (file_literal != nullptr) { | ||
| children[literal_child_idx] = std::move(file_literal); | ||
| } else { | ||
| children[struct_child_idx] = cast_file_expr_to_table_type(children[struct_child_idx], |
There was a problem hiding this comment.
This fallback still runs a non-lossless file-to-table cast below TableReader, so it can hide rows that normal materialization would reject. For example, take an old file STRUCT<a: BIGINT NOT NULL> mapped to a required table STRUCT<a: INT NOT NULL> and filter s.a = 1. Because BIGINT->INT is not lossless, rewrite_literal_to_file_type() returns null, but this branch still localizes the predicate as CAST(file_s.a AS INT) = 1. In non-strict mode an overflow value such as 2147483648 becomes NULL, the predicate drops that row, and _align_column_nullability() never sees the cast-produced NULL that the normal table materialization path would reject for the required INT child. Please keep this predicate at table level whenever the file-to-table leaf cast is not lossless, or otherwise prove the fallback cast cannot change the NULL/error behavior before filtering.
| */ | ||
| protected boolean isTableLevelCountStarPushdown() { | ||
| return pushDownAggNoGroupingOp == TPushAggOp.COUNT && pushDownCountSlotIds.isEmpty(); | ||
| } |
There was a problem hiding this comment.
This semantic COUNT() helper also needs to cover the other FE count-split shortcuts. Hive and TVF still check only getPushDownAggNoGroupingOp() == COUNT before calling FileSplitter.needSplitForCountPushdown(). When that returns false, they pass splittable=false to splitFile(), so a large splittable file becomes one full-file range. That was fine when the scan really used metadata only, but this PR makes non-empty COUNT arguments fall back to real row scans for unsafe mappings, filters/deletes, or old/unknown semantics. A Hive/TVF COUNT(col) fallback can therefore lose normal scan parallelism even though the result is still correct. Please gate those no-split shortcuts on the explicit COUNT()/COUNT(1) state, or keep normal splitting whenever pushDownCountSlotIds is non-empty.
TPC-H: Total hot run time: 29685 ms |
TPC-DS: Total hot run time: 177238 ms |
ClickBench: Total hot run time: 25.06 s |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 treated valid empty splits as scan failures, localized promoted nested struct predicates with mismatched operand types, and rejected whole Parquet schemas when unprojected leaves used unsupported logical types. Follow-up review also found that stop EOF could be counted as an empty file, unsupported requested leaves could reach metadata pruning before rejection, and reader-local page-cache range metadata lost warm non-exact reuse while lacking a bound. This change distinguishes stopped reads from empty splits, casts localized nested leaves back to table types, validates requested leaves before pruning and aggregate metadata, and shares bounded per-file range indexes through a bounded directory outside the ReadAt global hot path. Commented unit and regression cases cover empty CSV input, interrupted EOF, mixed Parquet struct leaf types, cross-reader non-exact cache reuse, bounded metadata, unprojected unsupported leaves, and an adjusted TIME_MILLIS predicate that statistics would otherwise prune.
A later review and external regression build exposed a separate COUNT pushdown ambiguity: COUNT(*) retained an arbitrary nullable scan slot after pruning, and BE inferred that placeholder as COUNT(column), producing 9015 instead of 10000 rows for special ORC data and 116 instead of 219 rows for Hive basic types. The fix now carries semantic COUNT arguments from Nereids through thrift to FileScannerV2; an empty argument list remains row-count semantics, while explicit COUNT(column) carries its mapped file column.
Release note
Fix FileScannerV2 compatibility for empty files, stopped reads, evolved nested predicates, and Parquet files containing unsupported logical columns; retain bounded cross-reader Parquet page-cache range reuse without a process-wide ReadAt lock. Also fix COUNT(*) aggregate pushdown when the planner-retained placeholder column contains NULL values.
Check List (For Author)