[fix](scanner)(nereids) Harden FileScannerV2 and fix external COUNT pushdown semantics#65548
Open
Gabriel39 wants to merge 17 commits into
Open
[fix](scanner)(nereids) Harden FileScannerV2 and fix external COUNT pushdown semantics#65548Gabriel39 wants to merge 17 commits into
Gabriel39 wants to merge 17 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
marked this pull request as ready for review
July 13, 2026 11:34
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 30262 ms |
Contributor
TPC-DS: Total hot run time: 180840 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
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.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29659 ms |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
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.
Contributor
TPC-H: Total hot run time: 29893 ms |
Contributor
TPC-DS: Total hot run time: 178193 ms |
Contributor
ClickBench: Total hot run time: 25.01 s |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 14, 2026
### 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
July 14, 2026 08:58
Issue Number: None Related PR: apache#65548 Problem Summary: Metadata COUNT pushdown could bypass schema-evolution casts and nullability validation for non-trivial column mappings. It also treated an omitted semantic-argument field from an old FE as explicit COUNT(*). Preserve thrift field presence through the scan path and only use footer COUNT(col) for a trivial direct mapping; otherwise use the normal materialization path. Fix external-file COUNT correctness and validation for schema-evolved columns during metadata pushdown. - Test: BE unit tests and regression test - ./run-be-ut.sh --run --filter="TableReaderTest.PushDownCount*" - ./run-regression-test.sh --run -d external_table_p0/tvf -s test_file_scanner_v2_review_fixes - Behavior changed: Yes; unsafe or ambiguous metadata COUNT plans now fall back to the normal scan path. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: External-file COUNT pushdown could discard CAST semantics, let table-level row counts answer COUNT(col), and let the legacy scanner reinterpret ambiguous or column-count plans as COUNT(*). Native files with a partial length prefix could also be mistaken for clean EOF. Restrict metadata COUNT to explicit COUNT(*) semantics at each planner and reader boundary, preserve CAST evaluation on normal file scans, and report truncated Native prefixes and bodies as corruption before allocation. ### Release note Fix external-file COUNT correctness across cast expressions, rolling-upgrade plans, and table-level metadata, and reject truncated Native blocks. ### Check List (For Author) - Test: BE unit tests - ./run-be-ut.sh --run --filter="FileScannerTest.V1CountPushdownRequiresExplicitCountStarArguments:NativeV2ReaderTest.RejectsPartialBlockLengthAsCorruption:TableReaderTest.TableLevelCountRequiresExplicitCountStarArguments" -j 4 - Behavior changed: Yes; unsafe COUNT metadata pushdown now falls back to normal scanning, and truncated Native input returns a corruption error. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: The Iceberg table-level COUNT unit test omitted the semantic COUNT argument field. After metadata COUNT was restricted to explicit COUNT(*) plans, the test fell back to position-delete scanning with intentionally omitted reader context and aborted on a missing split cache. Mark the test plan with an explicit empty COUNT argument list so it continues to exercise the assigned table-level row count path. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="IcebergV2ReaderTest.IcebergTableLevelCountUsesAssignedRowCountWithPositionDelete" -j 32 - Behavior changed: No; test-only correction. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: File-local predicate rewriting always cast an evolved nested struct leaf from its physical file type back to the table type. Although correct, this performed a conversion for every row even when the predicate literal could be represented exactly in the file type. Prefer rewriting binary and IN literals to the physical file leaf type when the file-to-table mapping is lossless and every literal round-trips exactly; otherwise retain the table literal and cast the file leaf. Clarify the separate COUNT(*) and COUNT(column) metadata semantics for both legacy and V2 file scanners. ### Release note Improve evolved nested struct predicate evaluation while preserving exact fallback semantics; clarify file COUNT pushdown behavior. ### Check List (For Author) - Test: Unit Test - ColumnMapperCastTest.* (23 tests passed) - FileScanner and TableReader COUNT pushdown tests (3 tests passed) - Behavior changed: Yes; safe nested predicates compare in the file type, while unsafe conversions fall back to table-type evaluation. - Does this need documentation: No
### 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
### 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
### 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
### 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
### What problem does this PR solve?\n\nFile-local struct predicates could hide lossy materialization errors, and Hive/TVF treated COUNT(column) like metadata-only COUNT(*).\n\nIssue Number: None\n\n### Release note\n\nNone\n\n### Check List\n\n- [x] Regression tests\n- [x] Code format
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: FileScannerV2 disabled adaptive batch sizing whenever the FE sent COUNT, even when TableReader rejected metadata COUNT because of multiple arguments, unsafe mappings, filters, or deletes. Those fallback scans materialized normal rows using the fixed runtime batch size, inflating block memory for wide columns. Track whether the active split actually emits metadata-derived COUNT rows and suppress adaptive sizing only in that case; fallback scans retain the first-batch probe and predictor updates. ### Release note None ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.AdaptiveBatchSizeRunsForCountFallbackOnly - TableReaderTest COUNT coverage - Behavior changed: No. Query results are unchanged; normal COUNT fallback scans retain adaptive batch sizing. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: HudiHybridReader and PaimonHybridReader forwarded the aggregate opcode to their active child readers but dropped the optional COUNT argument state. New BEs therefore treated explicit COUNT(*) and safe COUNT(column) plans as unknown old-FE semantics and fell back to full row scans. The hybrid wrappers also hid the child metadata-COUNT state from FileScannerV2, allowing synthetic aggregate rows to reach adaptive batch-size sampling. Forward COUNT arguments to both native and JNI children and delegate the active split metadata state through the hybrid reader. ### Release note None ### Check List (For Author) - Test: Unit Test - HudiHybridReaderTest.* - PaimonHybridReaderTest.* - FileScannerV2 and TableReader COUNT coverage - Behavior changed: No. Query results are unchanged; eligible Hudi and Paimon COUNT scans retain metadata aggregation and correct adaptive-batch accounting. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65548 Problem Summary: Nested fallback tests required the source column to stay in predicate_columns even though unsafe predicates remain table-level and readers may classify the required source as non-predicate. Verify the stable scan contract instead: the source column is scanned exactly once with the expected file-local id and no file conjunct is produced. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="ColumnMapperCastTest.NestedElementAtConjunctStaysTableLevelForNullableFileLeafMappedToRequiredTableLeaf:ColumnMapperCastTest.NestedElementAtConjunctStaysTableLevelForNonLosslessFileToTableCast" (2 tests passed) - ./run-be-ut.sh --run --filter="ColumnMapper*.*" (111 tests passed) - Behavior changed: No - Does this need documentation: No
Gabriel39
force-pushed
the
codex/fix-file-scanner-v2-review-issues
branch
from
July 17, 2026 15:28
0115333 to
5ecb493
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Found one blocking correctness issue in the supported BE-first rolling-upgrade path; details are inline. Current-version COUNT semantics and the scanner hardening otherwise look coherent after duplicate suppression.
Critical checkpoint conclusions:
- Goal and proof: The patch largely preserves COUNT(*)/COUNT(col) identity through Nereids, thrift, V1/V2, table readers, and hybrid readers, and hardens EOF, schema-evolution, unsupported-type, and cache paths. Unit coverage is broad, but no test covers an old FE that has already reduced Iceberg/Paimon splits before a new BE handles absent field 55; the inline issue shows the goal is not fully met.
- Scope and clarity: Changes are focused on scanner-v2 review fixes and COUNT semantics. The new absent/empty/non-empty protocol is clearly commented, but its upgrade invariant is incomplete.
- Concurrency: FE state is coordinator-local and scanner COUNT state is per operator/split. The shared Parquet page-cache directory and per-file indexes are independently bounded and mutex-protected; no reverse lock order or heavy I/O under those locks was found.
- Lifecycle: TableReader and hybrid metadata-count state reset per split. The page-cache directory is a function-local static with bounded strong references and no cycle; reader close releases reader-local ranges while shared metadata safely survives warm scans.
- Configuration: No configuration item was added.
- Compatibility: Current FE/current BE semantics are separated correctly, but old-FE/new-BE Iceberg/Paimon plans can silently undercount after irreversible FE split reduction. New-FE/old-BE ambiguity is already covered by the existing rolling-upgrade thread.
- Parallel paths: V1/V2, Hive/TVF, Iceberg/Paimon/Hudi, native/JNI, Parquet/ORC, table-level/file-level metadata, deletes, and adaptive fallback were checked. The accepted issue affects both V1 and V2; no additional split-truncation instance exists in Hive/TVF/Hudi.
- Conditional checks: COUNT argument absence, explicit empty, non-empty, casts, multiple arguments, non-trivial mappings, filters/runtime filters, and delete dependencies were traced. Current guards are sound except when nullopt arrives after old-FE table-format split loss.
- Test coverage: Added BE/FE unit tests cover transport, fallback, placeholders, malformed Native input, nested predicates, unsupported Parquet types, hybrid forwarding, and page-cache bounds. The mixed-version reduced-split case is missing. The TVF regression still does not reach planner COUNT pushdown, already reported in discussion r3593618409.
- Test results: The changed expected output is consistent with the fixture queries. Per the review-only contract, I did not build or run tests and am not independently confirming the author-reported runs.
- Observability: Existing scan profiles/counters cover metadata COUNT, adaptive behavior, empty files, and page cache; no new logging or metric is required for this patch.
- Transactions and persistence: Not involved.
- Data writes: Not involved; this is a read/planning path.
- FE-BE variables:
push_down_count_slot_idsis wired through current Nereids file scans and both BE scanner generations, with hybrid forwarding. The missing safe protocol is specifically the supported mixed old-FE/new-BE table-level plan. - Performance: Adaptive fallback, bounded cache metadata, warm subrange reuse, and no-split conditions were checked. No additional material regression remains at current head.
- Other correctness/error/memory checks: Nested nullability and lossless-cast gates preserve residual error behavior; unsupported requested Parquet leaves are rejected before pruning/aggregation; page-cache ownership and partial-miss fallback are safe. The remaining Native zero-byte read concern is the exact existing discussion r3578952073 and was not duplicated.
User focus: no additional focus was supplied, so the full authoritative change set was reviewed.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29476 ms |
Contributor
TPC-DS: Total hot run time: 179220 ms |
Contributor
ClickBench: Total hot run time: 25.47 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
approved these changes
Jul 18, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
4 participants
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
This PR fixes two correctness areas in external file scans.
1. Preserve external
COUNT(*)/COUNT(column)semanticsExternal
COUNT(*)retained an arbitrary scan slot after column pruning. BE then inferred aggregate semantics from that post-pruning scan shape and could treat the retained nullable placeholder as the argument ofCOUNT(column). This produced wrong results, including 9015 instead of 10000 rows for special ORC data and 116 instead of 219 rows for Hive basic types.The fix transports semantic COUNT arguments from Nereids through
TPlanNode.push_down_count_slot_idsto both file-scanner implementations and their table/hybrid readers. The thrift field has an explicit compatibility contract:COUNT(*)/ row-count semantics;COUNT(column)arguments, which may use metadata only when every mapping is proven safe.Nereids no longer applies storage COUNT pushdown to cast arguments whose null/error behavior would be lost. FE split planning also restricts Iceberg/Paimon metadata-count split reduction and Hive/TVF no-split shortcuts to explicit row-count semantics, so a
COUNT(column)fallback retains all data splits and normal scan parallelism. BE V1, V2, Hudi, Paimon, and table-reader paths preserve the same argument state, reject unsafe metadata shortcuts, and keep adaptive batching enabled for real-row fallbacks.2. Harden FileScannerV2 schema and reader edge cases
FileScannerV2 treated valid empty splits as failures, could count stopped EOF or malformed Native input as an empty file, localized evolved nested predicates without fully preserving type/nullability contracts, and rejected whole Parquet schemas when only unprojected leaves used unsupported logical types. Parquet page-cache range metadata also lost warm non-exact reuse while lacking a bound.
The fix distinguishes stopped reads, valid empty input, and malformed/truncated input; keeps unsafe nested predicates above
TableReader; validates only semantically required Parquet projections while preserving strict checks for real predicate andCOUNT(column)inputs; uses safe row-position/default carriers for placeholder-only paths; and shares bounded per-file page-range indexes without adding a process-wide lock to theReadAthot path.Release note
Fix wrong external
COUNT(*)/COUNT(column)results and unsafe metadata pushdown across Nereids, thrift, FE split planning, and BE file readers. Also harden FileScannerV2 handling of empty or interrupted input, evolved nested predicates, unsupported unprojected Parquet logical types, COUNT placeholders, hybrid readers, and bounded cross-reader page-cache range reuse.Check List (For Author)
ColumnMapper*.*BE ASAN suite (111 tests passed)test_special_orc_formatsandtest_hive_basic_typereproduce the fixed COUNT(*) wrong resultstest_file_scanner_v2_review_fixesregression suite covers empty input, evolved nested types, unsupported Parquet placeholders, and COUNT fallback behavior