[fix](be) Harden FileScannerV2 schema and reader edge cases#65548
Open
Gabriel39 wants to merge 17 commits into
Open
[fix](be) Harden FileScannerV2 schema and reader edge cases#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:
|
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
Contributor
FE Regression Coverage ReportIncrement line coverage |
### 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
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed current head 48ab9ca23554fe64eaf8cb06beaa96d571a7027f. One additional distinct performance issue remains: FileScannerV2 disables adaptive batch sizing even when the new COUNT guards fall back to materializing normal rows. See the inline comment.
Checkpoint conclusions:
- Goal and scope: the patch consistently hardens FileScannerV2 schema/reader edge cases and carries explicit COUNT semantics through the same-version FE-to-BE path; the changes remain focused on those behaviors and their tests.
- Correctness and lifecycle: current-head empty-split handling advances each range once; COUNT(*)/COUNT(col), placeholder, delete, unsupported-Parquet-type, and nested-predicate result semantics are coherent apart from concerns already recorded in existing threads.
- Concurrency and performance: the shared Parquet page-range directory is bounded, version-keyed, and mutex-protected without a reverse lock order found. The accepted inline finding covers the remaining COUNT-fallback batch-size/memory regression.
- Compatibility and parallel paths: same-version V1/V2, Hive/TVF, Iceberg, Paimon, Parquet, ORC, and Native paths were checked. The mixed-version COUNT/split-reduction risk remains covered by the existing rolling-upgrade discussion, so I did not duplicate it.
- Tests and observability: the patch adds targeted BE, FE, and regression coverage plus the
EmptyFileNumprofile counter. Per the review contract, I did not run local builds/tests; CI evidence should remain the validation source. - Configuration, persistence, transactions, and writes: no new configuration propagation, persisted state, transactional boundary, or data-write path is introduced.
There was no additional user-provided review focus. Existing unresolved review threads remain authoritative.
Contributor
FE UT Coverage ReportIncrement line coverage |
### 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
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29650 ms |
Contributor
There was a problem hiding this comment.
Request changes: two new P2 issues remain on the current head.
- Native Hudi COW COUNT plans reach FileScannerV2, but the new COUNT-argument state stops at the outer hybrid reader. The child therefore rejects metadata aggregation and turns footer/stripe COUNT into a full base-file scan.
- The new
local()TVF COUNT regression never activates storage-layer aggregation in production, so it cannot exercise the protocol or mapping fallback it claims to cover.
Critical checkpoint conclusions:
- Goal and proof: The patch fixes many FileScannerV2 schema, EOF, COUNT, and cache edge cases, but the goal is not fully proven because the Hudi wrapper loses the new state and the TVF COUNT regression is an ordinary scan.
- Scope and clarity: The changes are broad but cohesive around scanner-v2 hardening. Most safety conditions are documented; no unrelated code was found.
- Concurrency and thread safety: The shared Parquet cache directory uses bounded, per-file locked state and validates payload hits; lock work and eviction behavior did not reveal a new correctness or deadlock issue.
- Lifecycle and static state: Split prepare/abort/close and adaptive-state resets are balanced on direct readers, with no cross-TU initialization concern. The active-child lifecycle/state handoff missing from the Hudi hybrid is covered inline.
- Configuration: No new configuration item is introduced. Existing default-enabled FileScannerV2 and external COUNT pushdown make the Hudi regression reachable.
- Compatibility: Current-version FE-to-V1/direct-V2 COUNT semantics are consistent. Mixed-version ambiguity remains covered by the existing rolling-upgrade thread and was not duplicated here.
- Parallel paths and conditions: V1, direct V2, Hive, Iceberg, Paimon, Hudi, and TVF paths were traced. The new field is handled in direct paths but not the reachable Hudi child wrapper.
- Tests and results: The added unit/regression coverage is substantial and expected outputs are deterministic, but the TVF COUNT case does not reach the feature under test and there is no Hudi hybrid COUNT-state test. Per the review-task contract, this was a static review; no build or test command was run.
- Observability: New empty-file and adaptive counters are appropriate for the changed lifecycle. No additional logging/metric blocker was found.
- Transactions, persistence, and writes: Not applicable; these changes are read-path scanner/planner logic and introduce no persisted state or data mutation.
- FE-BE variables: Thrift serialization, local-state storage, ExprId/SlotId/global-index mapping, and V1/direct-V2 consumption were checked. The same state is not forwarded when Hudi reconstructs child
TableReadOptions. - Performance: The Hudi full-scan fallback is the new actionable regression. Cache lookup, mapper localization, and direct adaptive batching showed no other new issue.
- Other correctness: Placeholder/delete row alignment, unsupported Parquet leaves, nested nullability/casts, EOF/stop cleanup, and split ownership were rechecked. Remaining live concerns are already represented by existing review threads and were duplicate-suppressed.
Contributor
TPC-DS: Total hot run time: 177292 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
### 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
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29759 ms |
Contributor
There was a problem hiding this comment.
Completed a static review of head d74d2c3f00ee981b026f10bd0c7579e083ab5e09 against base ec716229c84bfd9a67c18c2e61913f651e02e1c4, including all 55 changed files, the required FileScannerV2 design material, and the existing review threads. I found no new nonduplicate actionable issue to add; remaining concerns are already represented by existing inline threads.
Checkpoint conclusions:
- Goal and result correctness: current-version planning preserves explicit COUNT semantics from Nereids ExprIds through Thrift SlotIds and V1/V2 scanner dispatch, with empty/non-empty/absent states kept distinct. Table-level and file-level metadata paths now use those semantics, and Hudi/Paimon hybrid readers forward/reset the active child state used by adaptive batching.
- Scanner and format paths: reviewed V1/V2, Hive/TVF/Iceberg/Paimon/Hudi, and Parquet/ORC/Native paths, including split reduction, placeholders, position/equality/deletion-vector dependencies, zero-row handling, unsupported Parquet logical leaves, and nested-struct predicate localization. The mixed-version COUNT protocol, Native zero-byte short-read EOF, and unreachable TVF COUNT regression remain covered by existing threads and were not duplicated.
- Concurrency and lifecycle: the shared Parquet page-cache directory and per-file range indices are bounded, snapshots are taken under the per-index lock, stale payload metadata falls back to a full base read, and no reverse lock order or stale-byte path was substantiated. Reader close/abort and hybrid split transitions reset or delegate split-scoped metadata state consistently; the function-local static directory has process lifetime without cross-translation-unit initialization dependencies.
- Scope and interfaces: changes are focused on scanner/planner/format behavior and tests. No new configuration, persistence, transaction/write, or security boundary applies. The new optional FE/BE field is safe for current/current execution; its rolling-upgrade limitation is the existing compatibility thread.
- Performance and observability: metadata COUNT no longer suppresses adaptive sizing on row-scan fallbacks, placeholder columns avoid unnecessary Parquet validation/decoding, page-cache discovery remains shared but bounded, and empty-file/adaptive counters follow the reviewed paths.
- Tests: the patch adds focused BE and FE unit coverage plus external-table regression fixtures. The TVF end-to-end COUNT case does not reach storage aggregate pushdown, as already reported. This review was static only: the prepared checkout has no initialized worktree dependencies or
thirdparty/installed/bin/protoc, and the review contract prohibited builds or source changes. The macOS BE-unit job currently fails before tests because the runner supplies Java 25 while Doris requires JDK 17.
No additional review focus was provided. Existing inline threads remain the source of truth for unresolved items.
Contributor
TPC-DS: Total hot run time: 177884 ms |
Contributor
ClickBench: Total hot run time: 25.26 s |
### 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
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29812 ms |
Contributor
There was a problem hiding this comment.
Automated review completed for head 011533388043153e9134c1f6925d3ef0b0f63302.
No new non-duplicate actionable finding or inline comment survived the full review and three convergence rounds. Mechanisms rediscovered during convergence were suppressed under strict root-cause deduplication; the existing inline discussions remain the source of truth.
Checkpoint conclusions:
- Intended behavior and proof: the patch consistently hardens FileScannerV2 empty/error handling, semantic COUNT transport and safe row fallback, complex predicate localization, unsupported Parquet projections, COUNT-star placeholders, hybrid-reader state, and bounded cross-reader page-range discovery. The changed tests cover the principal local contracts; known end-to-end coverage gaps are already raised inline.
- Scope and focus: all 55 changed files belong to those FE/BE/reader/test contracts. The supplied focus file adds no extra focus, and no unrelated production behavior was found.
- Concurrency, lifecycle, and memory safety: shared Parquet range-directory/index state is bounded and mutex-protected with no inverse lock order found; scanner/TableReader state is split-local and reset/delegated across prepare, abort, close, and hybrid children. No distinct stale-byte, ownership, buffer-boundary, use-after-free, or initialization-order issue survived deduplication.
- Configuration and compatibility: no new configuration or dynamic-update contract is introduced. The optional FE/BE COUNT field is conservative on a new BE receiving an old-FE plan; the remaining mixed-version capability limitation is already covered by existing inline feedback.
- Parallel and conditional paths: V1/V2, Hive/Iceberg/Paimon/Hudi/TVF, Parquet/ORC/Native/text/JNI, table/file metadata, deletes, runtime filters, primitive/complex projections, and metadata/row fallbacks were compared. New conditions correspond to concrete semantic proofs, and unsafe cases fall back or fail explicitly subject only to existing discussions.
- Tests and results: every changed unit/regression hunk, generated output, and fixture intent was reviewed for reachability and expected behavior. No local build or test was run, as required by the review bundle.
- Errors and observability: changed statuses are propagated or deliberately classified; stop EOF remains separate from valid empty input, and existing scan/cache profiles plus
EmptyFileNumremain connected. No separate high-value diagnostic gap was found. - Persistence and writes: no transaction, EditLog/replay, durable metadata, data mutation, atomic-write, or crash-recovery path is modified.
- FE/BE variable passing: COUNT arguments are captured as ExprIds, preserved by physical-plan copies, translated to scan SlotIds, serialized in the common file-scan path, consumed by both scanners, mapped to V2 GlobalIndices, and forwarded into hybrid readers. No distinct sender/consumer omission survived.
- Performance and other correctness: metadata COUNT and placeholder suppression avoid work only after semantic safety checks; adaptive sizing remains active for row fallbacks; cache metadata is bounded. No new non-duplicate correctness, lifecycle, memory-safety, compatibility, or material performance issue remained.
Validation was static because the review contract prohibited worktree setup and builds; .worktree_initialized and thirdparty/installed are absent. At submission time, compile, Cloud UT, vault regression, formatting, checkstyle, license, dependency, secret, and large-file checks were green. FE/BE UT and several regression/performance jobs were still pending. The macOS BE UT job failed during environment setup because Java 25 was selected while Doris requires Java 17, before project tests ran.
Contributor
TPC-DS: Total hot run time: 177433 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
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
2 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
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)