github-actions[bot] commented on code in PR #65496:
URL: https://github.com/apache/doris/pull/65496#discussion_r3565839710
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -624,8 +627,8 @@ void TableReader::_finalize_reader_condition_cache() {
_condition_cache_ctx = nullptr;
return;
}
- segment_v2::ConditionCache::instance()->insert(_condition_cache_key,
-
std::move(_condition_cache));
+ segment_v2::ConditionCache::instance()->insert(
+ _condition_cache_key, std::move(_condition_cache),
_condition_cache_ctx->base_granule);
Review Comment:
Persisting the cached base is not enough while the MISS bitmap still
includes the extra false guard bit allocated in
`_init_reader_condition_cache()`. For example, if the producer scan starts at
global granule B and covers only that granule, the cached vector has two bits:
bit 0 for B and the guard bit 1 left false. This new insert stores bit 1 with
`base_granule = B`, so a later hit whose pruned plan starts at B+1 treats that
guard as an in-range false bit; both V2 Parquet and V2 ORC only keep rows
conservatively once `cache_idx >= cache.size()`, so they can skip the whole
real granule B+1. Please either trim the guard before inserting, or store the
real covered granule count and make hit filtering treat the guard as outside
the authoritative bitmap.
##########
be/src/storage/segment/condition_cache.h:
##########
@@ -135,7 +138,8 @@ class ConditionCache : public LRUCachePolicy {
bool lookup(const KeyType& key, ConditionCacheHandle* handle);
template <typename KeyType>
- void insert(const KeyType& key, std::shared_ptr<std::vector<bool>>
filter_result);
+ void insert(const KeyType& key, std::shared_ptr<std::vector<bool>>
filter_result,
+ int64_t base_granule = 0);
Review Comment:
The default `base_granule = 0` leaves legacy `FileScanner` entries
indistinguishable from new base-aware V2 entries. The V1 scanner still inserts
with `cache->insert(_condition_cache_key, std::move(_condition_cache))`, so a
split whose reader marked bit 0 relative to a non-zero row-group/stripe base is
stored with base 0. A later V2 query for the same `ExternalCacheKey` trusts
`handle.get_base_granule()` and can filter with that wrong origin; the reverse
direction is also unsafe because V1 hits ignore the stored base and recompute
their current one. Since `enable_file_scanner_v2` is query-option driven, these
modes can alternate for the same cached file range. Please either update the
legacy scanner/readers to write and restore the stored base too, or version the
external cache key so base-aware and legacy coordinate systems cannot hit each
other.
--
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]
