github-actions[bot] commented on code in PR #65714:
URL: https://github.com/apache/doris/pull/65714#discussion_r3594744400
##########
be/src/format_v2/parquet/parquet_file_context.cpp:
##########
@@ -192,10 +192,12 @@ std::string build_page_cache_file_key(const
io::FileReader& file_reader,
const io::FileDescription&
file_description) {
const int64_t mtime =
file_description.mtime != 0 ? file_description.mtime :
file_reader.mtime();
- if (mtime == 0) {
- // StoragePageCache is process-global. A key with only path + unknown
mtime can outlive a
- // rewritten local test file, or any external file whose version was
not propagated. Disable
- // v2 parquet page cache until the scan descriptor carries a stable
object version.
+ if (mtime == 0 && !file_description.is_immutable) {
Review Comment:
[P1] Include the effective filesystem namespace in this cache key
This branch now enables the process-global page cache when an immutable file
has no mtime, but the resulting key still omits the actual filesystem instance.
S3 scan ranges leave `fs_name` unset, the endpoint is supplied separately
through scan properties, and `S3FileReader::path()` is only `s3://bucket/key`.
Two S3-compatible catalogs with different endpoints but the same bucket/key and
size will therefore collide, so the second scan can consume the first
endpoint's cached bytes and return wrong rows. HDFS has the analogous gap when
`FileDescription::fs_name` is empty and `FileFactory` falls back to
`hdfs_params.fs_name` only while opening the reader. Please include the
effective endpoint/nameservice identity in the key and cover two namespaces in
a test before enabling unknown-mtime reuse.
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -630,10 +632,11 @@ bool TableReader::_should_enable_condition_cache(const
FileScanRequest& file_req
!file_request.delete_conjuncts.empty()) {
return false;
}
- // Runtime filters can arrive late and their payload is not guaranteed to
be represented by the
- // scan-local digest. Without a read-only mode, a MISS could insert a
bitmap for P AND RF under
- // the digest for only P. This mirrors the old FileScanner guard.
- return !contains_runtime_filter(file_request.conjuncts);
+ // Only scanner-driven splits provide a digest rebuilt from the exact RF
snapshot. Keep the
+ // conservative behavior for standalone TableReader callers: their initial
digest may describe
+ // only static predicate P and must not store P AND RF under that key.
+ return _condition_cache_digest_covers_current_split ||
Review Comment:
[P1] Scope condition-cache entries to the storage namespace
This now enables condition-cache reuse for runtime-filter conjuncts, but the
external key below still contains only path, mtime, size, predicate digest, and
split range. S3 scan ranges do not carry `fs_name`; the endpoint is supplied
separately in scan properties. Native Iceberg/Hudi splits use mtime 0, and
Paimon's -1 is normalized to 0, so two catalogs on different endpoints with the
same `s3://bucket/key`, size, range, and RF payload will collide. The second
scan can then install the first endpoint's survivor bitmap and skip granules
that actually contain matches. Please add the effective endpoint/filesystem
identity to `ExternalCacheKey` (or disable this cache when it is unavailable)
and cover two namespaces in a regression test.
--
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]
