gavinchou commented on code in PR #65058:
URL: https://github.com/apache/doris/pull/65058#discussion_r3535969435
##########
be/src/io/cache/file_cache_common.h:
##########
@@ -163,6 +164,13 @@ struct CacheContext {
}
query_id = io_context->query_id ? *io_context->query_id : TUniqueId();
is_warmup = io_context->is_warmup;
+ remote_scan_cache_write_limiter =
io_context->remote_scan_cache_write_limiter;
+ admit_cache_write_by_remote_scan_limiter =
+ remote_scan_cache_write_limiter != nullptr &&
Review Comment:
This gate still misses external file scans because `FileScanner` /
`FileScannerV2` build their `IOContext` with query id, stats, and disposable
state, but never attach `remote_scan_cache_write_limiter` or mark the context
as `READER_QUERY`. External format readers can still use `FILE_BLOCK_CACHE`, so
a SELECT with `file_cache_query_limit_bytes = 0` can populate file cache
through external scans. Please attach the query limiter from
`RuntimeState::get_query_ctx()` and set `READER_QUERY` for query scans while
preserving load/non-query semantics.
##########
be/src/storage/index/inverted/inverted_index_fs_directory.cpp:
##########
@@ -184,10 +184,14 @@ void DorisFSDirectory::FSIndexInput::setIoContext(const
void* io_ctx) {
_io_ctx.reader_type = ctx->reader_type;
_io_ctx.query_id = ctx->query_id;
_io_ctx.file_cache_stats = ctx->file_cache_stats;
+ _io_ctx.file_cache_miss_policy = ctx->file_cache_miss_policy;
+ _io_ctx.remote_scan_cache_write_limiter =
ctx->remote_scan_cache_write_limiter;
Review Comment:
Copying the limiter here only helps callers that already put it into the
`IOContext`. The score-runtime statistics path in `OlapScanner` builds a
`CollectionStatistics::collect()` context with reader type, query id, stats,
and `is_inverted_index=true`, but never copies
`state->get_query_ctx()->remote_scan_cache_write_limiter()`.
`CollectionStatistics::process_segment()` then opens inverted-index files
through this null-limiter context, so `file_cache_query_limit_bytes=0` can
still admit file-cache writes. Please attach the query limiter to that
collection context as well.
##########
be/src/storage/segment/variant/variant_external_meta_reader.cpp:
##########
@@ -90,7 +90,9 @@ Status
VariantExternalMetaReader::init_from_footer(std::shared_ptr<const Segment
}
Status VariantExternalMetaReader::lookup_meta_by_path(const std::string&
rel_path,
- ColumnMetaPB* out_meta)
const {
+ ColumnMetaPB* out_meta,
+ OlapReaderStatistics*
stats,
+ const io::IOContext*
source_io_ctx) const {
Review Comment:
This threads the source context to the value-meta reads, but the key side of
external variant metadata is still outside the limiter. `init_from_footer()`
loads `_key_reader` with `_key_reader->load(true, false)`, and
`lookup_meta_by_path()` / `load_all()` build
`IndexedColumnIterator(_key_reader.get())` without stats or `source_io_ctx`.
Those `variant_meta_keys` pages can still reach `IndexedColumnReader` with a
default `IOContext`, so with `file_cache_query_limit_segment_meta=true` and
`file_cache_query_limit_bytes=0`, reading external variant keys can still write
file cache. Please thread the source context/stats into the key reader load and
key iterators too.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -5784,7 +5795,7 @@ public TQueryOptions toThrift() {
tResult.setIcebergWriteTargetFileSizeBytes(icebergWriteTargetFileSizeBytes);
tResult.setEnableLocalShufflePlanner(enableLocalShufflePlanner);
-
+ tResult.setFileCacheQueryLimitBytes(fileCacheQueryLimitBytes);
Review Comment:
`file_cache_query_limit_bytes` also needs to be refreshed for reusable
prepared short-circuit queries. The first execute serializes planner query
options into `ShortCircuitQueryContext`, while later reusable executes can
reuse cached options or just the cached UUID, and BE creates the point-query
limiter from the cached `RuntimeState` options. If a session first executes
with the default `-1`, then runs `SET file_cache_query_limit_bytes=0` and
re-executes the same prepared statement, BE can still see `-1` and skip limiter
creation. Please refresh/invalidate the short-circuit query options when this
session variable changes, or carry the current limit in the lightweight request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
