Repository navigation
[improvement](load) Add LoadTableReader for FileScannerV2 ingestion - #69333
Merged
Gabriel39 merged 3 commits intoOct 26, 2026
Merged
Conversation
### What problem does this PR solve? Forward-port apache#69320, including all review fixes through a73b750. Supported traditional loads use a TableReader subclass for source defaults, pre-filters, destination conversion, rejection accounting and partial updates. Adapt descriptor and reader APIs to master while preserving its query routing. ### Release note Route supported traditional file loads through FileScannerV2. ### Check List - Tests: carried over BE unit and Stream Load regression coverage; focused Clang syntax, clang-format 16 and build hygiene checks passed. - Full runtime testing remains for CI. - Behavior changed: Yes, supported traditional loads select V2. - Documentation: updated the FileScannerV2 design document. Source commits: abdba83, 439f82b, 2f46771, a73b750.
Gabriel39
requested review from
gavinchou,
liaoxin01,
morningman and
yiguolei
as code owners
October 20, 2026 08:19
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
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.
No actionable new defect was substantiated in the static review of all 24 changed paths at 6ece82b. Two complete-scope reviewers and a separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS in round 1. The review converged; no unresolved concrete candidate remains. There were no existing inline threads or P0/P1 findings to carry forward. No additional user focus was supplied.
The inspected path is FE source/destination tuple planning -> FileScannerV2 -> LoadTableReader -> TableColumnMapper and physical reader -> source defaults/presence/pre-filters -> destination conversion and validation -> Scanner destination predicates -> existing load sink. The three initial risks—cross-format binding/casts, presence/partial-update semantics, and malformed-record/pipe/counter handling—were checked against actual callers and legacy behavior and dismissed with evidence.
Critical checkpoint conclusions:
- Goal and evidence: the adapter implements traditional file-load conversion at the table-reader layer. Added BE and Stream Load cases exercise defaults, strict conversion, rejection statistics, missing keys, partial updates, pipe messages, and EOF; these tests were inspected, not executed.
- Scope and reuse: changes are focused on scanner selection, an ingestion adapter, physical rejection/presence metadata, tests, and documentation; common mapping and reader lifecycle are reused.
- Concurrency and locks: no new threads, locks, atomics, or lock ordering were introduced. New counters/masks are scanner-local; existing pipe synchronization remains responsible for producer/consumer coordination.
- Lifecycle and memory: presence masks are consumed before filtering and before reader close; source state is cleared on completion/abort/close. No new ownership cycle or static-initialization dependency was found. Const expressions are materialized before row checks.
- Configuration: no new configuration item exists. Supported traditional loads intentionally select V2 independently of the query option; query selection retains its existing gates.
- Compatibility: no persisted/wire representation changes. FE-generated nullable source types and defaults match the adapter; native Parquet/ORC values are mapped into source types before destination expressions. Legacy INT96 timezone behavior is retained when the FE omits the new timestamp contract. WAL recovery continues through its existing query/TVF reader.
- Thrift/protobuf: no gensrc definitions, field IDs, or enum values changed.
- Parallel paths: Avro/Arrow ingestion stays on its existing path; query readers retain rejection disabled by default. CSV/JSON and the advertised text/proto/native-format branches were traced.
- Conditions and errors: rejection branches have concrete malformed-record scenarios; non-recoverable statuses propagate. Rollback restores accepted-row frontiers, and destination filtering follows conversion/error accounting.
- Test coverage: focused unit/regression cases cover the main adapter and pipe boundaries. New integration fixtures concentrate on CSV/JSON; a full Parquet/ORC writer/encoding matrix, additional sequence/prefilter combinations, and chunked-transfer parse failures remain unexecuted coverage limits.
- Expected results: fixture-derived row values and loaded/filtered/unselected counts were checked; SELECT results are ordered. No .out files changed. The regression suite uses SQL assertions and final table cleanup, diverging from repository test conventions; this was not treated as a runtime defect.
- Observability: rejected and unselected totals use separate counters and incremental scanner publication. Existing reader/IO profiles remain; adapter conversion/default/prefilter subphase timing is less detailed than the legacy scanner.
- Transaction/persistence: no EditLog, transaction, recovery metadata, rowset/version, or publish protocol was modified.
- Data writes and atomicity: converted rows continue into the existing load sink/transaction path; no new commit boundary was introduced. Crash/failover behavior was not exercised.
- FE/BE propagation: no new transmitted variable was added; the existing source/destination tuple and expression protocol is reused.
- Performance: work and presence metadata are bounded by batch rows and projected columns, with expressions prepared once. No measured performance claim is made; benchmarks and final ELF/ABI validation were not run.
- Other concerns: non-object JSON outer-array recovery and empty arrays with jsonpaths retain legacy behavior; they were recorded as existing edge-case coverage gaps, not new findings.
Validation was static only, as required by this invocation. No builds, runtime tests, source edits, or performance measurements were performed. Convergence describes completion of this review process, not a runtime-validation result.
Review converged after 1 round(s).
Apply the regression assertion fixes from apache#69320 to its master port. Update UTF-8, physical field-count and JSON no-match diagnostics while preserving status, rejected-row and exact JSON source-record checks. Validation: all four affected Groovy assertion replays, all three suite syntax checks, repository clang-format 16 check, and diff checks passed. Full cluster regression execution remains for CI. (cherry picked from commit 7e29433)
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
Applied the same regression assertion fixes as #69320 to this master port. The source PR failures covered are:
Validation: the four affected Groovy assertions fail before the update and pass afterward in local assertion replays. The three reported cases use their recorded CI responses; the subsequent alias case uses the diagnostic from the reader's no-match branch. All three Groovy files pass syntax checks on master, and the repository clang-format 16 check passes. Full cluster regressions have not been rerun locally; CI has been requested again. |
Contributor
There was a problem hiding this comment.
No new actionable findings were verified for head ebd0f5f7820ebaf65f7abe35e251d134c661473b. Reviewed all 27 changed paths and the complete supplied diff. The main review and all three independent reviewers completed their rechecks with NO_NEW_VALUABLE_FINDINGS; the review converged in 1 round. There were no existing inline threads or additional user focus points.
The reviewed flow is FE source/default/expression planning → FileScannerV2 selection and split progression → physical decoding and column mapping → source materialization and missing-field handling → source pre-filters → destination conversion/strict/nullability checks → Scanner destination predicates → existing load quality/transaction handling.
Critical checkpoint conclusions:
- Goal and evidence: the adapter places ingestion conversion and validation in LoadTableReader while retaining the existing source/destination tuple protocol. Added unit and regression cases cover the principal contracts by inspection; runtime success is not established by this review.
- Scope and abstraction: dispatch, physical malformed-record rejection/presence, the finalization hook and load counters form a focused change. File identities are translated through the mapper.
- Concurrency and locks: new reader, presence and counter state is scanner-local. Pipe concurrency uses the existing locks and conditions; no new thread or lock-order change was identified.
- Lifecycle: source blocks, expressions and descriptors have query/reader ownership; presence metadata is consumed before physical-reader close. Empty converted batches, split transitions, abort and retryable close were checked. No new static-initialization dependency was identified.
- Configuration: no new configuration item. Supported traditional loads deliberately select by format; query selection retains its existing gates.
- Compatibility: current FE supplies both default channels, source metadata and expressions. Source casts, native-reader timestamp options and file/name identities were traced against the legacy path. No new stored representation or system dependency was identified.
- Thrift/protobuf: no definition, field-ID or enum changes.
- Parallel paths: query versus traditional/TVF load, Avro/Arrow fallback, Parquet/ORC, CSV/TEXT/PROTO, WAL recovery, JSON file/line/message/chunked modes, ordinary versus flexible updates were examined.
- Special conditions: field-count, encoding, missing-key/auto-increment, omitted-value/explicit-NULL and sequence-map checks have concrete triggering cases. No new incorrect condition was verified.
- Test coverage: inspected strict/non-strict, defaults, pre-filter ordering, malformed-record rollback, partial-update marks, EOF/batch/split and stream-message cases. The broader validation limits are listed below.
- Test results: fixture row/count expectations and changed diagnostics were checked statically; no .out files changed. No execution result is claimed.
- Observability: rejected and unselected rows remain separate, with incremental scanner reporting and existing error logging/profile hierarchy.
- Persistence and transactions: no journal, transaction or persistence protocol changed; existing load-quality acceptance remains downstream. Current WAL replay continues through its TVF query path.
- Writes and atomicity: physical rejection rolls back partial cells/presence before continuation; conversion removes invalid whole rows before downstream loading. No new write-atomicity defect was verified.
- FE/BE variables: no new transmitted variable; existing tuples/defaults/strict/sequence metadata supply the adapter. New presence and counters are local.
- Performance and memory: buffers and existing physical-reader fast paths remain in use; added presence/filter work is batch-related. No demonstrated pathological regression was found, and no performance or memory benchmark result is claimed.
- Final recheck: all initial risks and candidate-like concerns have evidence-based dispositions; no unresolved new defect or unreviewed changed path remains.
Validation limits: static analysis only, with no builds, tests or source edits. Added tests do not establish a full Parquet/ORC traditional-load differential matrix, compressed/TEXT/PROTO load integration coverage, or FE-to-BE flexible sequence/auto-increment integration. Empty outer arrays with jsonpaths and scalar outer-array rollback retain legacy behavior; the new simple-array rejection tests do not establish a universal array-error policy. These are explicit testing/existing-behavior limits, not verified new inline findings.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 29029 ms |
Contributor
TPC-DS: Total hot run time: 150480 ms |
Contributor
ClickBench: Total hot run time: 24.32 s |
Contributor
TPC-H: Total hot run time: 28350 ms |
Contributor
TPC-DS: Total hot run time: 150501 ms |
Contributor
ClickBench: Total hot run time: 24.25 s |
ADBC and Fluss do not support ordinary file loads. Reject their table markers before checking the physical format, since the default Parquet format otherwise admits them to LoadTableReader. Add shared selector coverage for an omitted option and default format, both option values and explicit physical formats. Query selection stays on the existing connector-specific path. Validation: the new regression fails before the fix; 31 targeted tests pass in the branch-4.2 ASAN/UBSAN harness afterward, including the two CI failure assertions. Master operator and relevant test translation units pass syntax checks. clang-format 16, build hygiene and diff checks pass. Full BE UT execution remains for CI.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
Fixed the two BE UT failures reported by build 1074847. The load selector previously accepted the default Parquet format before considering the ADBC/Fluss table marker. These connectors support queries only, so the load gate now rejects their markers before the physical-format switch. Query routing is unchanged. Added a shared regression covering both connectors with the default format/omitted option, both option values, and explicit physical formats. The same change is applied to #69320 and #69333. Validation:
The complete master BE UT suite was not run locally; CI has been requested again. |
4 of 8 tasks
Contributor
There was a problem hiding this comment.
Static review of #69333 at 2fae0d9. One P2 data-correctness/compatibility finding is reported inline: the newly mandatory V2 Parquet load path changes a supported non-NULL year-zero local timestamp to NULL. No P0/P1 finding or existing blocking inline thread was identified.
The main reviewer and two complete-scope reviewers covered all 27 changed paths; a separate reviewer investigated conversion provenance, presence/default/partial-update semantics, and EOF/counters/cleanup. The source/destination load protocol was traced from FE planning through scanner selection, table/column mapping, physical reads, source finalization, destination validation and the existing sink.
Applicable critical checkpoints:
- Goal and functional evidence: LoadTableReader places ingestion conversion at the table layer and retains destination filtering in Scanner. Added tests exercise the intended flow, but the inline Parquet compatibility defect prevents full preservation of load behavior.
- Scope and clarity: changes are focused on dispatch, the adapter/finalization contract, physical rejection/presence metadata, counters, documentation and tests.
- Concurrency and locks: new mutable reader/IOContext state is scanner-local; existing synchronized pipes handle concurrent producers. No new thread, lock order or deadlock path was found.
- Lifecycle/static initialization: missing masks are consumed before reader close; EOF with rows, fully rejected batches, split transitions, abort and close were traced. No new owner cycle or cross-TU initialization dependency was found.
- Configuration: no configuration item was added. The dispatch intentionally ignores the query V2 option for supported loads; load strict_mode and query enable_strict_cast are separate, relevant to the finding.
- Compatibility/internal interfaces: finalization's changed schema/row-count contract was checked across callers. The Parquet input compatibility issue is inline; no wire or persisted format changed.
- Thrift/protobuf IDs: no generated protocol definition changed; branch field-ID comparison is not applicable.
- Parallel paths: CSV, JSON object/path/root/array, TEXT/PROTO, Parquet/ORC and WAL selection were traced; Avro/Arrow keep their existing load path and normal WAL TVF replay stays separate.
- Special conditions: wrong field counts, UTF8 rejection, optional defaults, explicit NULL, flexible keys/auto-increment/sequence, query-only connectors and EOF conditions were checked against upstream producers and legacy behavior. Inherited JSON edge behavior was not reposted as a new defect.
- Test coverage: selector, adapter, malformed-record/default/bitmap and pipe tests plus regression fixtures were inspected. There is no added end-to-end binary-format load matrix or complete Scanner-to-load-counter runtime test.
- Test results: new expected rows/counts and changed diagnostics are statically consistent with the code. No new .out file was added. No runtime pass is claimed.
- Observability: rejected/unselected counters retain separate channels into load statistics and the profile hierarchy remains. The finding exposes a conversion failure that escapes rejected-row accounting.
- Transactions/persistence/failover: no EditLog, durable registration, commit, replay, visible-version or MoW publication protocol is changed.
- Data writes/atomicity/crashes: the reader feeds existing transactional sinks; no new commit/rollback or crash-recovery path was introduced. The inline finding can change the value delivered to that sink.
- FE-BE variables: existing tuples/required slots/defaults/expressions are reused; current FE writes both default representations and preserves slot order. No new transmitted variable was added.
- Performance/memory: batch masks, validity filters, source/default materialization and buffer ownership were inspected; no additional substantiated performance or memory defect was found. No benchmark claim is made.
- Other issues and focus: no further distinct verified defect or unresolved candidate remains. There was no additional user-provided focus; every initial risk has an explicit ledger disposition.
Validation and coverage limits: this invocation was static-only by contract; no builds, tests, cluster runs or source edits were performed. Added tests do not establish runtime correctness for the full Parquet/ORC/PROTO/TEXT/compressed/quoted/nonzero-split/cancellation matrix. These runtime limits remain even after static reviewer convergence. Review uses three bounded rounds; the trusted helper records the convergence and delivery result.
Review converged after 3 round(s).
| // Load planners omit query session options. Thrift's default makes an omitted V2 | ||
| // option indistinguishable from false, so ingestion selects by format instead. | ||
| switch (scan_params.format_type) { | ||
| case TFileFormatType::FORMAT_PARQUET: |
Contributor
There was a problem hiding this comment.
[P2] Preserve year-zero timestamps when enabling V2 Parquet loads
Selecting V2 for every Parquet load turns supported year-zero local timestamps into NULL. For example, an optional INT64 TIMESTAMP(MICROS, isAdjustedToUTC=false) value -62167219200000000 loaded into a nullable STRING column previously produced 0000-01-01 00:00:00.000000 through V1's Int64ToTimestamp and CAST. V2's parquet_timestamp_micros rejects values before 0001, and its non-strict native decoder marks this non-NULL physical value as NULL. LoadTableReader::_convert then sees an already-NULL source, so strict_mode=true still accepts/stores NULL without counting a rejected row; a required destination instead rejects the formerly valid row. Preserve the prior load behavior for Doris' supported year-zero values before routing these loads unconditionally.
Contributor
TPC-H: Total hot run time: 29020 ms |
Contributor
TPC-DS: Total hot run time: 150829 ms |
Contributor
ClickBench: Total hot run time: 24.31 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Oct 26, 2026
liaoxin01
approved these changes
Oct 26, 2026
morningman
added a commit
to morningman/doris
that referenced
this pull request
Oct 26, 2026
…e Lance reader ### What problem does this PR solve? Issue Number: None Related PR: apache#69581 Problem Summary: BE configure runs `build-support/check-build-hygiene.sh`. Its reverse reach budget caps how many TUs may transitively include `util/rle_encoding.h` at the baseline 823 (master 9a48f81, 2026-08-18) plus 10%, that is 906. Master 9af1840 is at 903 (apache#69333 added the load table reader and its tests), and the Lance reader adds the same four TUs that the `util/threadpool.h` rebaseline in this PR counts (`format_v2/table/lance_reader.cpp`, `format_v2/lance/lance_runtime_filter_helper.cpp` and their unit tests), so configure on master plus this PR fails with "util/rle_encoding.h now reaches 907 TUs (baseline 823 +10% = 906)". Before apache#69333 the PR stayed under the limit, which is why the earlier rebaseline only had to cover `util/threadpool.h`. The four TUs add no new edge. `exprs/vexpr_context.h` includes `storage/segment/column_reader.h`, which includes `storage/segment/parsed_page.h` -> `util/rle_encoding.h`, the path the other format_v2 readers already take. As for `util/threadpool.h`, this rebaselines `util/rle_encoding.h` to the measured 907 (limit 998) and records the reason next to the entry; cutting the `vexpr_context.h` -> `column_reader.h` edge stays the separate build cleanup and would lower both. ### Release note None ### Check List (For Author) - Test: No need to test: build configuration only. `bash build-support/check-build-hygiene.sh` passes on master 9af1840 plus this PR. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3 of 12 tasks
2 of 6 tasks
sollhui
added a commit
that referenced
this pull request
Oct 29, 2026
#69716) ### What problem does this PR solve? Issue Number: None Related PR: #69333 Problem Summary: After CSV ingestion switched to FileScannerV2, `DelimitedTextReader` opened remote files directly through `FileFactory`, bypassing the sequential prefetch used by the original CSV reader. S3 reads therefore block the scanner instead of overlapping remote I/O with CSV parsing. Restore `DelegateReader::create_file_reader(SEQUENTIAL)` in the shared CSV/Text reader, passing the shared IOContext and adjusted split range. Unknown-size first splits use an end sentinel bounded by the physical file size. Tracing remains outside the prefetch layer, and shared stream-load pipes retain their existing ownership and cancellation behavior. To safely integrate asynchronous reads with V2 scanner statistics: - Each prefetch buffer owns its cache statistics and an IOContext snapshot. The consumer merges completed statistics exactly once before consuming, reusing or closing the buffer. Workers never update the scanner's cache-statistics object. - Close drains active reads even after a foreground timeout, keeping borrowed scanner state alive until I/O completes. Queued tasks observe CLOSED and skip reading. Cleanup can consequently take longer than `buffered_reader_read_timeout_ms` while an underlying request is outstanding. - The ring modulus uses allocated capacity: a 6 MiB setting allocates one 4 MiB buffer and wraps at 4 MiB. Settings below 4 MiB use one buffer; the 16 MiB default is unchanged. - CSV/Text filesystem readers close before collecting prefetch profile counters. Repeated close does not publish counters twice. ### Performance For the TPC-H dataset load benchmark, elapsed time decreased from **2617.33 s** to **1696.06 s**, saving **921.27 s**. This is a **35.2% reduction in load time**, or approximately **1.54x speedup**, for the measured workload. | Dataset | Before | After | Load time reduction | Speedup | | --- | ---: | ---: | ---: | ---: | | TPC-H | 2617.33 s | 1696.06 s | 35.2% | 1.54x | ### Release note Restore asynchronous S3 prefetch for FileScannerV2 CSV and text reads, with safe cache-statistics aggregation and cleanup, consistent ring capacity and populated prefetch profiles. ### Check List (For Author) - Test: - Added S3 mock coverage for whole-file, mid-row split and unknown-size reads; checks row values, outer tracing, background S3 reads, prefetch profile publication and repeated close. - Added buffered-reader tests for four concurrent workers with isolated cache statistics, partial consumption, slot reuse, unconsumed-buffer collection, successful reads finishing after foreground timeout, explicit close/destruction, and exact bytes across 0/1/6/10/16 MiB ring settings from zero/nonzero offsets. Added a shared stream-pipe ownership test. - Passed Clang 16 syntax checking of the complete `buffered_reader_test.cpp` translation unit against repository and installed third-party headers with `-Werror -Wunused-member-function`. - Passed repository clang-format 16 formatting/check scripts, `build-support/check-build-hygiene.sh`, and `git diff --check`. - Full BE build and runtime tests are **not verified locally**: `./run-be-ut.sh --run --filter='BufferedReaderTest.*:CsvV2ReaderTest.*' -j 8` stops at JDK initialization (`Failed setting boot class path`). Separate production/CSV syntax attempts encounter installed C++ library and stale generated-header incompatibilities. No compilation database is available for clang-tidy. Runtime and sanitizer validation remain pending CI. - The S3 mock test disables file cache; the concurrent statistics test exercises the actual prefetch implementation with a mock reader updating `FileCacheStatistics`. No cached-S3 end-to-end runtime result is claimed. - TPC-H load benchmark: elapsed time decreased from 2617.33 s to 1696.06 s (35.2% less load time; approximately 1.54x speedup). - Behavior changed: - [x] Yes. Eligible S3 CSV/Text readers use sequential prefetch. Shared prefetch readers isolate cache counters, drain outstanding reads during cleanup and normalize ring capacity. CSV/Text publishes prefetch profiles on close. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
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.
What problem does this PR solve?
Forward-port the complete changes from #69320 to
master, including all review fixes througha73b75002fa40f3df7c8189d8fe3560382d2e13a.Add
LoadTableReaderso supported traditional file loads use FileScannerV2 with the existing FE/Thrift source and destination tuples. Source defaults, pre-filters, destination conversion, strict/nullability checks, and flexible partial-update bitmaps run in the table-reader finalization hook. CSV/JSON malformed records are rejected with rollback and load counters. JSON pipes consume every message until EOF; empty arrays and invalid configured roots contribute to the load quality gate without losing valid siblings.Master-specific adaptations:
TableReader::refresh_conjuncts()andRowDescriptorconstructors, and useSlotDescriptor::type().Avro and Arrow IPC loads keep their existing path. No FE or Thrift changes. This port is independent of #69312.
Validation
build-support/check-build-hygiene.shandgit diff --cached --check.table_reader.cpp, using this master's generated headers and UT build configuration. The standalone scanner check suppresses two pre-existing unused-constant warnings from master.run-be-ut.shcompleted configuration and started the full build. The fresh build requires about 14,750 targets, so it was stopped and replaced with the focused syntax checks above. No master runtime unit-test or cluster-regression pass is claimed; full build and runtime validation remain for CI.Release note
Route supported traditional file loads through FileScannerV2 with a table reader for load conversion and validation.