Uh oh!
There was an error while loading. Please reload this page.
[improvement](be) Limit packed file writes to rowset first segment - #62342
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Only merge the first segment in a rowset into packed files. Later segments now bypass packed files so large imports and memory-pressure-triggered multi-segment flushes do not keep feeding packed-file buffering. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - Unit Test: Attempted ./run-be-ut.sh --run --filter=PackedFileSystemTest.* -j 8, but the build is currently blocked by an unrelated missing header faiss/invlists/PreadInvertedLists.h in be/src/storage/index/ann/faiss_ann_index.cpp - Manual test: Ran clang++ -fsyntax-only for be/src/io/fs/packed_file_system.cpp and be/test/io/fs/packed_file_system_test.cpp - Behavior changed: Yes (packed file now applies only to the first segment in a rowset) - Does this need documentation: No
Thearas
commented
Apr 10, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
liaoxin01
commented
Apr 10, 2026
run buildall |
There was a problem hiding this comment.
Pull request overview
This PR changes BE packed-file behavior so only the first segment of a rowset is eligible for packed-file aggregation, while later segments write directly to the underlying filesystem to avoid prolonged buffering during large imports or multi-segment flushes.
Changes:
- Add filename-based gating in
PackedFileSystem::create_file_impl()to bypassPackedFileWriterfor non-first segments. - Introduce helper logic to detect “first segment” based on
.dat/.idxfilenames. - Add unit tests asserting packed vs direct writer selection for segment 0 vs later segments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
be/src/io/fs/packed_file_system.cpp | Adds should_use_packed_writer() and conditionally skips PackedFileWriter for later segments. |
be/test/io/fs/packed_file_system_test.cpp | Adds tests to validate packed-writer usage for first segment and direct-writer usage for later segments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: apache#62342 Problem Summary: Clarify in code comments that should_use_packed_writer() only handles V2 index file names because V1 inverted-index tablets are excluded before PackedFileSystem is enabled. ### Release note None ### Check List (For Author) - Test: No need to test (comment-only change) - Behavior changed: No - Does this need documentation: No
liaoxin01
commented
Apr 10, 2026
run buildall |
hello-stephen
commented
Apr 10, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 10, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
liaoxin01
commented
Apr 15, 2026
/review |
There was a problem hiding this comment.
Found 1 blocking issue.
be/src/io/fs/packed_file_system.cpp:should_use_packed_writer()only inspectsfile.filename(). For cloudStorageResource::remote_segment_path()withpath_version == 1, segment paths aredata/{shard}/{tablet_id}/{rowset_id}/{seg_id}.datand V2 index paths are.../{rowset_id}/{seg_id}.idx. The basename becomes just1.dat/1.idx, sorfind('_')fails and the helper returnstrue, meaning later segments still get wrapped inPackedFileWriter. That leaves the multi-segment cloud path unchanged and the PR goal is not met on this supported path format. Please parse the segment-id decision from the full path or otherwise handlepath_version == 1, and add a regression/unit test for that path shape.
Critical checkpoint conclusions:
- Goal and proof: Partially accomplished. The new tests prove the behavior only for local-style names; they do not cover the supported cloud remote path v1 format where the change currently fails.
- Change scope: Small and focused, but the filename heuristic is too narrow for all supported rowset path shapes.
- Concurrency: No new concurrency or lock-safety concerns in this patch.
- Lifecycle/static init: No special lifecycle or static initialization concerns introduced.
- Config: No configuration changes.
- Compatibility: No storage-format or symbol compatibility changes.
- Parallel paths: Not fully handled. Local/path_version0 names and cloud path_version1 names are functionally parallel here, but only the former is covered.
- Special conditions: The new conditional lacks coverage for the non-intuitive remote path layout.
- Test coverage: Added BE unit tests are useful but incomplete; missing remote path_version1 cases for both
.datand V2.idx. - Observability: Existing logging is sufficient; no additional observability appears necessary.
- Transaction/persistence: No direct persistence protocol change, but wrong writer selection changes whether packed-slice metadata is produced.
- Data write correctness: The intended write-path behavior change is not guaranteed for all supported cloud paths.
- FE/BE variable passing: Not applicable.
- Performance: The intended memory-usage improvement is not realized on the affected cloud path.
- Other issues: None beyond the path-format gap above.
Uh oh!
There was an error while loading. Please reload this page.
V1 inverted-index tablets are excluded before that in be/src/storage/rowset/rowset_writer_context.h, where packed_file_active is forced off for V1.
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
…62342) Only merge the first segment in a rowset into packed files. Later segments bypass packed files so large imports and memory-pressure-triggered multi-segment flushes do not keep feeding packed-file buffering.
…62342) Only merge the first segment in a rowset into packed files. Later segments bypass packed files so large imports and memory-pressure-triggered multi-segment flushes do not keep feeding packed-file buffering.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Only merge the first segment in a rowset into packed files. Later segments bypass packed files so large imports and memory-pressure-triggered multi-segment flushes do not keep feeding packed-file buffering.
Release note
None
Check List (For Author)