[fix](file cache) guard null IOContext in cached remote reader#63842
Merged
eldenmoon merged 1 commit intoMay 29, 2026
Merged
Conversation
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31270 ms |
Contributor
TPC-DS: Total hot run time: 172213 ms |
eldenmoon
requested review from
Gabriel39,
gavinchou and
morningman
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Guards CachedRemoteFileReader::read_at_impl against a null IOContext by substituting a default-constructed one, and forwards NativeReader::_io_ctx to its header/block file reads to avoid passing null down to the cached remote reader.
Changes:
- Allocate a stack default
IOContextwhen caller passesnullptrinCachedRemoteFileReader::read_at_impl. - Thread
_io_ctxthroughNativeReaderheader validation and block length/body reads. - Add a BE unit test exercising
CachedRemoteFileReader::read_atwith no explicitIOContext.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| be/src/io/cache/cached_remote_file_reader.cpp | Substitute default IOContext when caller passes null before dereferencing fields. |
| be/src/format/native/native_reader.cpp | Forward _io_ctx to header validation and block length/body read_at calls. |
| be/test/io/cache/block_file_cache_test.cpp | Adds cached_remote_file_reader_accepts_null_io_context test covering null-context path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
approved these changes
May 28, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Member
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.
Automated review completed for PR 63842. I did not find any blocking correctness issues in the changed code.
Critical checkpoint conclusions:
- Goal and tests: The PR addresses the stated null IOContext path by defaulting CachedRemoteFileReader::read_at_impl to a local IOContext and by forwarding NativeReader::_io_ctx into header/block reads. The added BE unit test covers the direct null-IOContext cached reader case.
- Scope and clarity: The change is small and focused on IOContext propagation/null handling.
- Concurrency: No new shared mutable state or lock ordering was introduced. The stack default IOContext is used synchronously within read_at_impl.
- Lifecycle/static initialization: No new static/global lifecycle hazards were introduced.
- Configuration/compatibility: No new configuration, protocol, storage format, or rolling-upgrade compatibility concerns.
- Parallel paths: The relevant NativeReader reads now pass IOContext, and CachedRemoteFileReader tolerates the public FileReader::read_at default null context.
- Conditional checks: The new null handling is straightforward and matches FileReader::read_at API semantics.
- Test coverage: Added targeted BE unit coverage. I did not rerun the full BE UT suite; I ran git diff --check locally in this runner.
- Observability: Existing file-cache statistics/profile behavior is preserved when a caller provides IOContext; null context uses default stats behavior without query profile updates.
- Transaction/persistence/data write correctness: Not applicable to this PR.
- FE/BE variable passing: Not applicable.
- Performance: No meaningful hot-path regression found; the default IOContext construction is only a small stack object and only affects read calls.
- User focus: No additional user-provided review focus was specified.
github-actions Bot
pushed a commit
that referenced
this pull request
May 29, 2026
- Guard `CachedRemoteFileReader::read_at_impl` against nullable `IOContext`. - Pass `NativeReader` `_io_ctx` through header and block reads. - Add BE unit coverage for reading through `CachedRemoteFileReader` without an explicit `IOContext`.
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.
Proposed changes
CachedRemoteFileReader::read_at_implagainst nullableIOContext.NativeReader_io_ctxthrough header and block reads.CachedRemoteFileReaderwithout an explicitIOContext.Root cause
FileReader::read_atallows callers to omitIOContext, butCachedRemoteFileReader::read_at_impldereferenced it before checking.NativeReaderalso issued header and block reads without forwarding its_io_ctx, which exposed the cached remote reader to a null context.Test
git diff --checkDORIS_CLANG_HOME=/mnt/disk6/common/ldb_toolchain_026 DORIS_THIRDPARTY=/mnt/disk6/common/doris-thirdparties/doris-thirdparty-master ./run-be-ut.sh --run --filter='BlockFileCacheTest.cached_remote_file_reader_accepts_null_io_context'