[improvement](be) Add ORC v2 stripe merged reads#65478
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one blocking compatibility regression in the ORC v2 stripe merged read path and left it inline.
Critical checkpoint conclusions:
- Goal/fit: the PR adds ORC v2 stripe-level merged reads and the implementation is otherwise focused on that path.
- Configuration/compatibility:
orc_once_max_read_bytes = 0is accepted by FE and tolerated by the existing v1 ORC path, but the new v2 input stream asserts during initialization. This is the submitted issue. - Parallel paths: compared the new v2 behavior with the existing v1 ORC merged-read path; the zero-value behavior is inconsistent.
- Lifecycle/concurrency/profile: reviewed stream ownership,
beforeReadStripe()flush behavior, and tracing/profile wrapping; no separate substantiated issue found. - Tests: the PR adds input-stream unit coverage and a reader integration/profile test, but it does not cover the accepted zero-value session config. I did not run BE tests in this checkout because
.worktree_initializedandthirdparty/installedare missing. - User focus: no additional user-provided focus points were present.
Subagent conclusions: OPT-1 and TSC-1 both identified the same orc_once_max_read_bytes = 0 compatibility regression and were merged into inline comment MAIN-1. No other candidates remained after duplicate merging. Convergence round 1 ended with both optimizer-rewrite and tests-session-config replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29536 ms |
TPC-DS: Total hot run time: 182992 ms |
ClickBench: Total hot run time: 25.02 s |
xylaaaaa
force-pushed
the
pr/orc-v2-stripe-prefetch-clean
branch
from
2d2fdba to
fa4dad3
Compare
|
/review |
1 similar comment
|
/review |
|
run buildall |
There was a problem hiding this comment.
Reviewed the ORC v2 stripe merged-read change. I found one blocking issue: the new merged-range cache uses untracked std::vector<char> storage for query-sized read spans, bypassing Doris memory tracking. I submitted one inline comment.
Critical checkpoints:
- Goal/test coverage: the PR implements ORC v2 stripe stream collection and merged small-stream reads; tests cover merge/direct/zero-value/profile paths, but allocator-aware memory accounting is still missing.
- Scope: the implementation is focused on the ORC v2 input stream and reader wiring.
- Concurrency/lifecycle: I did not find new shared cross-thread state; active cluster readers are flushed on stripe transition and destruction. The substantiated issue is memory accounting, not a race.
- Configuration/session compatibility:
orc_once_max_read_bytesandorc_max_merge_distance_bytesare forwarded to BE. The earlierorc_once_max_read_bytes = 0crash is already covered by an existing thread and current head now allows zero. - Parallel paths/compatibility: the existing v1 ORC merge helper uses Doris allocator-aware storage for the same kind of cache; the v2 path should preserve that memory-accounting behavior.
- Error handling: changed read paths check
Statusand convert read failures to ORC parse errors at the expected boundary. - Performance/observability: merged-read profile counters were added, but the merged cache allocation itself is not tracked by Doris memory accounting.
- Tests: I did not run BE unit tests in this checkout because
thirdparty/installedandprotocare missing;git diff --checkpassed. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions:
optimizer-rewritereportedOPT-001as a duplicate ofMAIN-001; no optimizer/rewrite issue was found.tests-session-configreportedTSC-1as a duplicate ofMAIN-001; no separate tests/session/config issue was found.- Convergence round 1 ended with both live subagents returning
NO_NEW_VALUABLE_FINDINGSfor the same final comment set.
| } | ||
|
|
||
| const size_t range_size = _range.end_offset - _range.start_offset; | ||
| _cache.resize(range_size); |
There was a problem hiding this comment.
This cache should use Doris allocator-aware storage instead of a plain std::vector<char>. _load() resizes _cache to the full merged span, which is capped by orc_once_max_read_bytes; that session variable is only lower-bounded at 0 in FE and is forwarded directly to BE, so one scan can allocate multi-megabyte buffers per active merged cluster here. The existing v1 ORC merge reader keeps the same kind of cache in DorisUniqueBufferPtr<char>, which charges the allocation through Doris' memory accounting. Please switch this cache to the Doris allocator path, for example the same DorisUniqueBufferPtr<char> / make_unique_buffer pattern, so ORC v2 merged reads cannot bypass query memory tracking.
xylaaaaa
force-pushed
the
pr/orc-v2-stripe-prefetch-clean
branch
from
fa4dad3 to
5eab058
Compare
|
/review |
There was a problem hiding this comment.
Automated review completed. I found one additional blocking issue: ORC v2 now derives natural_read_size from the mutable BE config orc_natural_read_size_mb without validating it, so an accepted zero value can abort BE during ORC v2 reader initialization.
Existing inline threads already cover zero orc_once_max_read_bytes behavior and allocator-aware storage for the merged-range cache, so I did not duplicate those comments.
Critical checkpoint conclusions: the PR implements the intended ORC v2 small-stream merge path and keeps the change focused; CMake globs include the new production and test files; no new optimizer/rewrite, transaction, persistence, FE/BE protocol, or concurrency issue was found; lifecycle/profile flushing was reviewed around stripe switch and reader teardown; configuration review found the new natural-read-size validation gap; tests cover merge/direct/zero-once/profile behavior but do not cover invalid orc_natural_read_size_mb; validation here was static review plus git diff --check because this checkout lacks thirdparty/installed and thirdparty/installed/bin/protoc.
| auto input_stream = std::make_unique<DorisOrcInputStream>(_file_description->path, | ||
| _tracing_file_reader, _io_ctx.get()); | ||
| OrcFileInputStreamOptions input_stream_options; | ||
| input_stream_options.natural_read_size = config::orc_natural_read_size_mb << 20; |
There was a problem hiding this comment.
This makes ORC v2 depend on a mutable BE config value that is not validated before it reaches the new constructor check. orc_natural_read_size_mb is declared with DEFINE_mInt32, so /api/update_config can accept orc_natural_read_size_mb=0, and this line then derives natural_read_size = 0; immediately after that, OrcFileInputStream aborts on DORIS_CHECK_GT(_options.natural_read_size, 0) during reader initialization. That turns an accepted runtime config into a BE process crash for any ORC v2 scan. Please add a validator for a safe positive range, and compute the byte value after validation with a safe cast, or reject/report the invalid value before constructing the input stream.
xylaaaaa
force-pushed
the
pr/orc-v2-stripe-prefetch-clean
branch
from
5eab058 to
530ae71
Compare
|
run buildall |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ORC file scanner v2 used a thin ORC input stream that issued reads directly through the underlying file reader. It did not implement the v1 ORC stripe-level stream collection and small-range merge path, so remote object storage scans could regress to many small stream reads for wide ORC files, multi-stripe files, and lazy or predicate-driven scans. This change adds a v2-native ORC input stream that implements beforeReadStripe(), builds selected stream ranges, merges adjacent small streams with Doris PrefetchRange policy, and serves arbitrary repeated or backward stream reads from an immutable merged-range cache. Large streams and unmerged single streams continue to read directly. The reader is wired into ORC v2 without changing v1 or falling back to v1.
### Release note
Improve ORC file scanner v2 remote read performance by merging selected small stripe stream reads.
### Check List (For Author)
- Test: Unit Test / Manual test / Static analysis
- Unit Test: ./run-be-ut.sh --run --filter='OrcFileInputStreamTest.*:NewOrcReaderTest.StripePrefetchPublishesMergedReadProfile' -j 8 (passed, reported by user)
- Manual test: git diff --check (passed)
- Manual test: build-support/check-format.sh (passed)
- Manual test: ninja -C be/ut_build_ASAN src/format/CMakeFiles/Format.dir/__/format_v2/orc/orc_file_input_stream.cpp.o test/CMakeFiles/doris_be_test.dir/format_v2/orc/orc_file_input_stream_test.cpp.o (passed)
- Static analysis: build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN --files ... (attempted; own signed/unsigned diagnostic fixed; remaining failures are pre-existing/toolchain diagnostics in be/src/core/types.h and be/src/io/fs/buffered_reader.h)
- Behavior changed: Yes. ORC v2 now merges selected small stripe streams and reads each merged cluster through a cached span instead of issuing one remote read per small stream request.
- Does this need documentation: No
xylaaaaa
force-pushed
the
pr/orc-v2-stripe-prefetch-clean
branch
from
530ae71 to
cd574c6
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ORC file scanner v2 used a thin ORC input stream that issued reads directly through the underlying file reader. It did not implement the v1 ORC stripe-level stream collection and small-range merge path, so remote object storage scans could regress to many small stream reads for wide ORC files, multi-stripe files, and lazy or predicate-driven scans. This change adds a v2-native ORC input stream that implements beforeReadStripe(), builds selected stream ranges, merges adjacent small streams with Doris PrefetchRange policy, and serves arbitrary repeated or backward stream reads from an immutable merged-range cache. Large streams and unmerged single streams continue to read directly. The reader is wired into ORC v2 without changing v1 or falling back to v1.
Release note
Improve ORC file scanner v2 remote read performance by merging selected small stripe stream reads.
Check List (For Author)