Repository navigation
[perf](csv reader) Reduce V2 CSV parsing overhead - #69875
Merged
Gabriel39 merged 2 commits intoOct 30, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69716 Problem Summary: After restoring sequential prefetch, large V2 CSV loads still spend substantial CPU time in row parsing and string materialization. The reader compares a general separator at every byte, updates atomic profile counters per cell, starts three stage timers per row, and repeats checked string-column casts per cell. It also ignores the row cap supplied by TableReader. This change combines four optimizations on master: - Compare the separator byte directly for plain single-character separators; retain enclosed, multi-character, Hive/OpenCSV and PROTO parsing paths. - Accumulate exact work counters locally and publish at get_block/count/schema return, including errors. Sample stage timings every 1024 physical read_line calls, starting with the first, and expose unscaled SampledReadLineTime, SampledSplitLineTime and SampledDeserializeTime with sample counts. DelimitedTextReader remains full elapsed time; split-prefix alignment has its own full timer. - Validate nullable string column types and reserve offsets/null maps once per mutable batch, then insert strings directly with the existing NULL, quoting and escape semantics. - Honor FileReader::set_batch_size, retaining the existing byte budget and runtime default when no explicit cap is supplied. The caller still controls the maximum batch size; this does not force 16352-row blocks. Shared counters and batch sizing also apply to the Hive text reader. No speedup is claimed yet; the combined patch is ready for a controlled benchmark against its master parent. ### Release note Reduce V2 CSV parsing overhead and honor caller-provided batch sizes. Delimited-text per-stage profile timers are now explicitly named Sampled* and contain unscaled sampled durations; use the full DelimitedTextReader timer for elapsed-time comparisons. ### Check List (For Author) - Test - [x] Unit Test: added cases for dynamic/zero batch caps, byte limits, filtered blocks, cross-batch sampling and exact counters, single/multi-character separators, NULL and escape behavior, missing fields, reordered projection and copy-on-write. Extended error, rejected-row and COUNT counter coverage. - Execution: run-be-ut.sh was attempted with CsvV2ReaderTest.*, TextV2ReaderTest.* and LegacyAndV2/PlainCsvSplitTest.*. It exits before C++ compilation because the local JDK 17 fails to initialize (Failed setting boot class path), including outside the sandbox. Full build/UT and performance benchmark remain pending. - Passed: repository clang-format 16 formatting/check scripts, build-support/check-build-hygiene.sh and git diff --check. - Behavior changed: Yes. Existing batch-size contract is honored, and detailed stage timers are explicitly sampled. CSV value semantics are unchanged. - Does this need documentation: No. The profile includes a timing explanation; no new configuration or SQL syntax. Benchmark: use identical hardware, input, compression, cache conditions and load batch/byte-budget settings for the parent and this commit. Compare total load time, Scanner CPU time, DelimitedTextReader time and RowsRead/BlockNumber. Do not directly compare Sampled* timers with the previous full stage timers.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
sollhui
changed the title
![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 defect found in the supplied diff for head 08c5b57. All six changed files and their upstream/downstream reader contracts were reviewed. No existing inline findings or additional user focus points were present.
Critical checkpoint conclusions:
- Goal and proof: the implementation removes repeated separator/SerDe/type-check/profile work and honors caller batch caps. Focused BE tests cover the intended behavior; their code and expectations were inspected, not executed.
- Scope and clarity: changes stay within CSV/shared delimited readers and their tests, using existing FileReader hooks and column APIs.
- Concurrency and locks: pending counters, sample state and batch caps remain reader-local in serialized scanner calls. Profile publication uses existing atomic counters; prefetch workers do not access the new fields. No new locks or lock-order changes.
- Lifecycle and memory: column preparation follows recursive Block copy-on-write detachment. Both concrete column types are checked before unchecked cell casts; no view is retained across reads. Reserves use existing tracked column storage. No new static initialization dependency or ownership cycle.
- Configuration: no new settings; existing row limits, adaptive byte budgets and text attributes remain in force.
- Compatibility: no on-disk, external text syntax, protocol, dependency or toolchain change. Profile names and timing semantics deliberately change and are documented. No new libc API use was found; ELF compatibility was not measured.
- Thrift/protobuf: no definitions changed, so field-ID/rolling-upgrade comparisons are inapplicable.
- Parallel paths: reviewed plain single/multiple-byte separators, enclosed CSV, OpenCSV, PROTO, compressed CSV, Hive Text, generic non-string decoding, COUNT and schema discovery. Dispatch and layer ownership remain intact.
- Conditions: the separator specialization preserves exact delimiter advancement. NULL/empty/quoted-marker decisions retain their prior order; direct unescaping/insertion and NULL appends match the replaced operations.
- Test coverage: changing/zero caps, byte stops, filtered empty batches, cross-batch sampling, rejected rows, UTF-8 errors, COUNT, missing/reordered fields and shared-column ownership are covered by inspected tests. Relevant split, quote, compression and empty-projection tests were also inspected.
- Test results: added GTest expectations agree with the traced paths; no result fixture changed. This review supplies no executed-test result.
- Observability: exact work counters flush at block/count/schema API exits, including errors. Sampled durations are explicitly unscaled, sample counts are exposed, and full elapsed/filter/split-prefix timing remains available.
- Persistence and failover: no EditLog, transaction, version-visibility or delete-bitmap changes.
- Writes and atomicity: changes append in-memory scan columns only. Existing malformed-row rollback and status propagation remain; persistent write/crash atomicity is unaffected.
- FE/BE propagation: no new transmitted variables or sender changes.
- Performance: repeated hot-path work is removed and readers persist when caps change. Eager reservation is bounded by caller predictions/runtime limits; no concrete regression was established. Speedup and peak-memory effects require measurement.
- Other concerns: file-local mapping, residual/delete filtering and table-level casts/defaults retain their existing layer boundaries; no additional verified issue remains.
Validation limits: static review only; no builds, tests, ASAN, benchmarks, formatting/static-analysis tools or ELF audits were run. New tests do not directly add PROTO or nested-subcolumn COW fixtures; their equivalent operations and ownership were checked statically. No quantitative performance claim is made. These dynamic validation limits remain even after static review convergence.
Review converged after 2 rounds. Every assigned reviewer in the final round reported NO_NEW_VALUABLE_FINDINGS, required static coverage is complete, and all candidate/risk checks have evidence-backed dispositions. No unresolved static question remains.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: None Related PR: apache#69875 Problem Summary: BE UT build 1086461 aborts in StringFastPathPreservesValuesForSingleAndMultiByteSeparators during reader->open(). The new test passes a null RuntimeProfile, while NewPlainTextLineReader registers counters unconditionally. Use the fixture-owned profile, whose lifetime covers every reader, so the separator/NULL/escape/COW assertions can run. ### Release note None ### Check List (For Author) - Test: inspected the ASAN stack and constructor contract; repository clang-format 16, format check, build hygiene and git diff --check pass. Attempted the focused run-be-ut.sh test, but local JDK startup still fails before C++ build/test execution. CI rerun remains required. - Behavior changed: No production behavior change; correct test setup only. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28891 ms |
Contributor
TPC-DS: Total hot run time: 151171 ms |
Contributor
ClickBench: Total hot run time: 24.24 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
approved these changes
Oct 30, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
yiguolei
pushed a commit
to sollhui/doris
that referenced
this pull request
Nov 12, 2026
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?
Issue Number: None
Related PR: #69716
Problem Summary:
After restoring sequential prefetch, large V2 CSV loads still spend substantial CPU time in row parsing and string materialization. The reader compares a general separator at every byte, updates atomic profile counters per cell, starts three stage timers per row, and repeats checked string-column casts per cell. It also ignores the row cap supplied by TableReader.
This change combines four optimizations on master:
Shared counters and batch sizing also apply to the Hive text reader.
Performance benchmark
On TPC-DS SF100, elapsed time decreased from 1,944 seconds to 1,375 seconds:
Release note
Reduce V2 CSV parsing overhead and honor caller-provided batch sizes. Delimited-text per-stage profile timers are now explicitly named Sampled* and contain unscaled sampled durations; use the full DelimitedTextReader timer for elapsed-time comparisons.
Check List (For Author)
For further benchmark comparisons, use identical hardware, input, compression, cache conditions and load batch/byte-budget settings for the parent and this commit. Compare total load time, Scanner CPU time, DelimitedTextReader time and RowsRead/BlockNumber. Do not directly compare Sampled* timers with the previous full stage timers.