Repository navigation
[fix](packed-file) Honor write_file_cache option for packed small files - #69693
Merged
liaoxin01 merged 1 commit intoOct 28, 2026
Merged
Conversation
liaoxin01
requested review from
Gabriel39,
gavinchou,
luwei16 and
morningman
as code owners
October 27, 2026 13:12
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
liaoxin01
force-pushed
the
fix-packed-file-cache-write-switch
branch
from
October 27, 2026 13:27
6f8c77a to
96b3c41
Compare
PackedFileSystem only copied FileWriterOptions::write_file_cache into PackedAppendContext when enable_file_cache_write_index_file_only was on. Otherwise the append context kept its default true, so packed small files were always written to the NORMAL file cache queue, even for a load with disable_file_cache=true. Always follow the caller's option when one is given, so packed and direct writes make the same cache decision.
liaoxin01
force-pushed
the
fix-packed-file-cache-write-switch
branch
from
October 27, 2026 13:35
96b3c41 to
16669e7
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #69693 at 16669e74d12391f08b767b6e28fa8b2600237903: no new actionable findings. There are no existing inline findings to carry forward. All four changed files, the relevant producers/consumers, and the required review checkpoints were covered; the complete reviewer and separate risk reviewer both returned NO_NEW_VALUABLE_FINDINGS in round 1.
Checkpoint conclusions:
- Goal and evidence: the caller's false/true cache preference now reaches PackedAppendContext outside index-only mode and controls PackedFileManager's per-slice cache gate. The new unit test directly checks false, true, and null-option propagation; it does not exercise physical cache writes.
- Scope and reuse: the predicate simplification, unused include removal, and test-accessor cleanup are small and consistent with the existing interfaces.
- Concurrency and locking: each created writer receives its own copied context; shared filesystem policy is not mutated. Existing manager locking, task submission, and lock order are unchanged, with no additional heavy work under locks.
- Lifecycle and static initialization: options are consumed during creation; writer ownership, asynchronous data captures, buffer release, and upload completion remain unchanged. No new globals, cross-TU initialization dependency, or ownership cycle is introduced.
- Configuration: no configuration is added. Index-only overrides remain in RowsetWriterContext::get_file_writer_options; null options preserve the append-context policy.
- Compatibility: no production layout, virtual interface, stored format, or rolling-upgrade protocol changes. The removed accessor was BE_TEST-only; the affected test target enables -fno-access-control, and no accessor callers remain. No new libc API is introduced; no binary ABI validation was performed.
- Thrift/protobuf: no definitions or field IDs change.
- Parallel paths: later segments and threshold fallback retain the original inner-writer options; V1 index exclusion, direct delete-bitmap packing, and read-side wrapping remain consistent. The only nonzero rowset adaptive-size producer is compaction, which disables packing, so no reachable adaptive-cache regression was found. Cloud schema change can use packing and now honors output_to_file_cache; the PR description's broader claim that schema change disables packing does not match that path.
- Conditional checks and comments: the remaining null-options check has a supported fallback, and the nearby comment describes the concrete disable_file_cache failure. No speculative error guard is added.
- Test coverage: targeted propagation coverage complements existing load-cache and packed index-only regressions. Related segment-routing and buffer-release tests remain applicable. Discovered index-only test mutations restore their saved configuration. No deterministic new end-to-end cache test is added, and the existing load-cache regression randomly selects packing.
- Test results: no golden/result files change; the capacity assertions retain the accessor's exact meaning. Test execution was not performed.
- Observability: existing cache metrics, async task/byte counters, and failure logs cover this optional cache work; no additional instrumentation is required for this change.
- Transactions and persistence: visibility versions, delete bitmaps, rowset metadata, prepare/commit, and failover behavior are unchanged; this flag controls optional cache population.
- Writes, errors, and crash behavior: authoritative remote append/upload and checked Status propagation remain intact. Skipping the best-effort cache task introduces no new atomicity, deadlock, or crash-cleanup path.
- FE-BE variables: no new variable or sending path is introduced; the existing caller preference is consumed by the packed path.
- Performance and memory: the existing context copy gains one boolean assignment. Disabling caching avoids the optional data copy/task and cache allocation; no new significant allocation or tracking obligation is introduced.
- Module and other checks: public filesystem dispatch still uses FILESYSTEM_M. No reader-cache redesign, hub include addition, PCH/gate-table/template change, or test inclusion of source .cpp files is introduced. No additional issue emerged from the final special-case sweep.
No additional user focus was supplied. R1 (cache policy/adaptive/special paths) and R2 (private test access/global configuration) were independently verified and dismissed with source evidence. The final sweep found no unresolved in-scope candidate or static coverage gap. The review converged after 1 round.
Validation limit: static inspection only. No builds, tests, benchmarks, runtime cache validation, or release-artifact checks were run; optional external TDE implementations are outside the available checkout.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28479 ms |
Contributor
TPC-DS: Total hot run time: 151476 ms |
Contributor
ClickBench: Total hot run time: 24.45 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
bobhan1
approved these changes
Oct 28, 2026
gavinchou
approved these changes
Oct 28, 2026
yiguolei
pushed a commit
that referenced
this pull request
Nov 4, 2026
…ed small files #69693 (#70114) ### What problem does this PR solve? Issue Number: None Related PR: #69693 Problem Summary: Backport #69693 to `branch-4.2`. With packed files enabled, a load using `disable_file_cache=true` could still populate the file cache because the packed writer ignored the caller's option. Always propagate `FileWriterOptions::write_file_cache` to the packed append context. The test insertion conflict was resolved without importing the unrelated nonzero-first-segment feature/test, which is absent on this branch. Includes the cache-option unit test and preserves the original accessor cleanup. ### Release note Fix packed-file loads with `disable_file_cache=true` unexpectedly populating the file cache. ### Check List (For Author) - Test: clang-format 16 and `git diff --check` passed. BE unit tests are included but were not run locally: the test entry point was blocked by the Mac's unaccepted Xcode license. CI `run buildall` is requested separately after PR creation. - Behavior changed: Yes. Packed writes now honor the caller's file-cache setting. - Does this need documentation: No.
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: #59892, #60411, #64995
Problem Summary:
When packed file is enabled, the first segment file (
.dat) and the index file (.idx) of a rowset are written throughPackedFileSystem. The packed path decides whether to cache each small file fromPackedAppendContext::write_file_cache, which defaults totrue.RowsetWriterContextnever sets this field.PackedFileSystem::create_file_implonly copied the caller'sFileWriterOptions::write_file_cacheinto the append context whenenable_file_cache_write_index_file_onlywas on. With the default config, the packed path ignored the caller's choice and always wrote the small files to the NORMAL file cache queue.So a load with
disable_file_cache=truestill filled the NORMAL queue whenever packed file was enabled. Files written directly to S3 honor the same flag and skip the cache.test_load_cacheturnsenable_packed_fileon or off at random, so it fails intermittently with:The 237288 bytes are exactly the total size of the three packed segment files written by the load.
This PR always copies
opts->write_file_cacheinto the append context when the caller passes options. Packed and direct writes now make the same cache decision. Whenoptsis null, the packed path keeps thewrite_file_cachevalue already in the append context.Other callers are not affected:
RowsetWriterContext::get_file_writer_options, so with write-index-only enabled the result is the same as before.allow_packed_file=false, so adaptive file cache write is not affected.DeleteBitmapFileWriterbuildsPackedFileWriterdirectly, so it is not affected.This PR also removes the
BE_TEST-only accessorPackedFileWriter::buffer_capacity_for_test(). BE unit tests are compiled with-fno-access-control, so tests read private members directly.Release note
Fix loads with
disable_file_cache=truestill writing data to the file cache when packed file is enabled.Check List (For Author)
Test
Behavior changed:
write_file_cache=false(for example a load withdisable_file_cache=true) no longer populate the file cache.Does this need documentation?
Check List (For Reviewer who merge this PR)