[chore](be) Add FileScannerV2 review guidance#65475
Open
Gabriel39 wants to merge 9 commits into
Open
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Add directory-scoped review guidance for format_v2 so code reviews consistently check reader lifecycle, schema mapping, filtering and delete semantics, format boundaries, performance, and focused test coverage. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only review guidance) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Extend the format_v2 review guide with explicit external compatibility checks across lake formats, file formats, writer implementations, versions, type semantics, capability detection, fallbacks, and interoperability testing. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only review guidance) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Extend the format_v2 review guide with architecture and interface-contract checks derived from the FileScannerV2 design. Define the responsibility boundaries between Scanner, TableReader, TableColumnMapper, and FileReader, and identify concrete coupling patterns that reviews must reject. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only review guidance) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Convert the FileScannerV2 architecture reference into a repository-local Markdown document, preserving its diagrams and design contracts, and update the format_v2 review guide to use the local reference instead of a Feishu URL. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only change) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Add a repository-local Markdown reference for the FileScannerV2 Parquet scan pipeline and extend the format_v2 review guide with focused checks for Row Group, Page, and Row filtering, index safety, pruning cost, lazy materialization, observability, and differential correctness coverage. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only change) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Extend the format_v2 review guide with focused ORC checks for Doris predicate to SDK SearchArgument conversion, literal and cast correctness, Split and Stripe selection, row-index and Bloom-filter usage, lazy materialization, pruning overhead, observability, and differential correctness tests. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only change) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Add a mandatory cross-format FileReader review guide covering native index implementations, predicate and residual filtering, data and condition caches, physical and table virtual columns, performance observability, and differential correctness tests. Move detailed Parquet and ORC checklists out of AGENTS.md and retain mandatory local references to stay below the Codex instruction-size limit. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only change) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Translate the repository-local FileScannerV2 overview and Parquet scan design documents from Chinese to English, including headings, tables, callouts, Mermaid diagrams, troubleshooting guidance, and design contracts. Keep filenames and AGENTS.md references unchanged. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only translation; Markdown and character scans passed) - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29139 ms |
Contributor
TPC-DS: Total hot run time: 180458 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
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.
Reviewed PR #65475 at head 152f05c5299847a965e7ab0d4f688dd3e792be2b.
I reviewed the four add-only files:
be/src/format_v2/AGENTS.mddocs/file-scanner-v2-code-review-guide.mddocs/file-scanner-v2-design.mddocs/file-scanner-v2-parquet-scan-design.md
I did not find a substantiated blocking issue to raise inline. The new format-v2 review guide and linked design notes are consistent with the current FileScannerV2/TableReader/TableColumnMapper boundary, file-local/global identity model, conservative pruning and residual-filter requirements, condition-cache EOF/delete/runtime-filter safeguards, row-group/page planning flow, aggregate-pushdown constraints, and existing be/test/format_v2/ test layout.
Validation note: this runner is a shallow checkout and is missing .worktree_initialized and thirdparty/installed, so I treated this as a static review and did not run BE/FE builds or unit tests. The changed Markdown links were checked against the working tree.
Critical checkpoint conclusions:
- Goal/test proof: the PR adds review/design documentation only; the added files are present, cross-linked, and aligned with current format-v2 code paths. No runtime behavior change or required new product test was identified.
- Scope/focus: the change is focused on FileScannerV2 review guidance and architecture notes.
- Concurrency/lifecycle: no code concurrency or lifecycle behavior is changed; the guidance calls out split isolation, cleanup, cache publication, and reader lifecycle review points.
- Config/compatibility/persistence: no new config, storage format, protocol, or persistence behavior is introduced.
- Parallel paths: Parquet/ORC/JNI/text/JSON/native and table-reader boundary guidance was checked against the current V2 dispatch and reader contracts.
- Testing/CI: no executable tests were added because this is docs/review guidance; paths and Markdown links were checked, and
git show --checkreported no whitespace problems for the changed files. - Performance/observability: no runtime performance path is changed; the guidance preserves profile/cache/pruning observability expectations.
User focus: no additional user-provided focus points were supplied.
Subagent conclusions: optimizer-rewrite reported no candidate findings for optimizer/rewrite semantic-equivalence concerns. tests-session-config reported no candidate findings for tests, expected outputs, session/config propagation, compatibility, links, or likely CI/style failures. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Remove four superseded format_v2 and Parquet design documents after consolidating the active architecture and review guidance into the new repository-local FileScannerV2 documents. ### Release note None ### Check List (For Author) - Test: No need to test (documentation-only deletion; staged diff check passed) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary:
I reviewed the full PR 65475 diff and relevant format_v2 code paths. I did not find any actionable correctness, compatibility, or documentation-link issue that needs an inline review comment.
Critical checkpoint conclusions:
- Goal and proof: the PR adds directory-scoped FileScannerV2 review guidance and replacement design/review docs. The changed docs cover layer boundaries, schema mapping/materialization, predicate/delete/index pushdown, Parquet/ORC checks, compatibility, and test expectations. No runtime test is required for the docs-only change.
- Scope and focus: the change is focused on review guidance and design references. The deleted older planning docs are no longer referenced, and the new docs preserve the review-relevant API boundaries, identity rules, pruning/caching guidance, and test coverage expectations.
- Concurrency/lifecycle: no runtime code or lifecycle behavior changes. The lifecycle guidance was checked against the visible FileScannerV2/TableReader/ParquetReader flow.
- Config/session/protocol/persistence: no config item, session variable, FE-BE protocol, storage format, or persistence behavior is changed.
- Parallel code paths: the guide explicitly covers common FileReader, Parquet, ORC, JNI, text/CSV/JSON, and external table-format paths. I did not find a misleading omission that would create a review defect.
- Tests and expected outputs: no tests were run because this is documentation-only. I did run static checks for whitespace, relative Markdown link targets, balanced code fences, deleted-doc references, changed-file scope, and exact PR head/base metadata.
- Performance/observability: no runtime counters or metrics changed. The added guidance calls out pruning order, cache semantics, profile counters, and performance review risks.
- User focus: the focus file contained no additional user-provided focus points.
Subagent conclusions:
optimizer-rewrite: no candidates became inline comments; the subagent reported no optimizer/rewrite, semantic-equivalence, aggregate, or parallel-path issue.tests-session-config: no candidates became inline comments; the subagent reported no test guidance, expected-output, session/config, compatibility, stale-reference, or Markdown/style issue.- Final convergence Round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same current ledger and empty proposed final comment set.
Validation notes:
- Verified PR head stayed at
4be9a4ccd6700686d62a33c82593f7dfaa0a0d08. - Existing inline review context was empty.
- Local base ancestry was initially missing, so I fetched the exact base SHA and used GitHub PR metadata plus local base-to-head checks for review scope.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28975 ms |
Contributor
TPC-DS: Total hot run time: 180138 ms |
Contributor
ClickBench: Total hot run time: 24.96 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: Code reviews under
be/src/format_v2need persistent, directory-scoped guidance for the FileScannerV2 architecture and external-data correctness boundaries. This PR adds a localAGENTS.mdand repository-local design/review references covering:Detailed checklists are stored under
docs/and loaded on demand from the directory-scoped instructions to stay below the Codex instruction-size limit.Release note
None
Check List (For Author)