LuciferYang commented on code in PR #68018:
URL: https://github.com/apache/doris/pull/68018#discussion_r4033662802
##########
be/src/storage/segment/column_reader_cache.cpp:
##########
@@ -98,8 +107,12 @@ Status ColumnReaderCache::get_column_reader(int32_t col_uid,
OlapReaderStatistics* stats,
const io::IOContext* source_io_ctx,
std::optional<Field> const_value) {
- // Attempt to find in cache
- if (auto cached = _lookup({col_uid, {}})) {
+ // A caller that passes const_value reads a column whose on-disk value is
a placeholder, so it
+ // must not be served the on-disk reader that a caller without const_value
cached earlier: that
+ // reader would hand back the placeholder both as row data and as a zone
map. Fall through and
+ // build the constant reader, replacing the cached entry so later callers
get the real value too.
+ if (auto cached = _lookup({col_uid, {}});
+ cached != nullptr && (!const_value.has_value() ||
cached->is_constant())) {
Review Comment:
Removed the `ConstantColumnReader::new_index_iterator` override from this
PR, and this thread is the reason. Making the reader refuse index init based on
its dynamic type cannot be complete: cache eviction and a concurrent bare miss
(the two sibling P1s here) both rebuild a physical reader for the same column,
and the index path passes no constant, so the guard is bypassed. Residency and
arrival order can't carry a correctness property.
The reliable signal is the schema: the column's ordinal is commit-TSO /
version / binlog-TSO under known substitution conditions. #67995 makes
`Segment::new_index_iterator` (and the zone-map builders and the pushed-agg
gate) skip or synthesize based on that, which covers all three orderings at
once and fixes the pre-existing single-slot exposure too. Moving the whole
placeholder concern there rather than shipping a partial reader-type guard here.
This PR is now scoped to the cache contract only: a request carrying a
constant is not dropped on a hit, and the entry is replaced in place. That part
is complete and tested on its own.
##########
be/src/storage/segment/column_reader_cache.cpp:
##########
@@ -60,6 +60,15 @@ std::shared_ptr<ColumnReader>
ColumnReaderCache::_lookup(const ColumnReaderCache
void ColumnReaderCache::_insert_locked_nocheck(const ColumnReaderCacheKey& key,
const
std::shared_ptr<ColumnReader>& reader) {
+ // Replacing an existing key updates its node in place. Pushing a second
node for the same key
+ // would leave the first one unreachable in the list while eviction erases
the map entry of
+ // whichever copy reaches the tail, dropping the live reader from the map.
+ if (auto it = _cache_map.find(key); it != _cache_map.end()) {
Review Comment:
Removed the `ConstantColumnReader::new_index_iterator` override from this
PR, and this thread is the reason. Making the reader refuse index init based on
its dynamic type cannot be complete: cache eviction and a concurrent bare miss
(the two sibling P1s here) both rebuild a physical reader for the same column,
and the index path passes no constant, so the guard is bypassed. Residency and
arrival order can't carry a correctness property.
The reliable signal is the schema: the column's ordinal is commit-TSO /
version / binlog-TSO under known substitution conditions. #67995 makes
`Segment::new_index_iterator` (and the zone-map builders and the pushed-agg
gate) skip or synthesize based on that, which covers all three orderings at
once and fixes the pre-existing single-slot exposure too. Moving the whole
placeholder concern there rather than shipping a partial reader-type guard here.
This PR is now scoped to the cache contract only: a request carrying a
constant is not dropped on a hit, and the entry is replaced in place. That part
is complete and tested on its own.
##########
be/src/storage/segment/column_reader.h:
##########
@@ -1097,6 +1103,18 @@ class ConstantColumnReader : public ColumnReader {
Status get_segment_zone_map(segment_v2::ZoneMap* zone_map) const override;
+ // This reader serves a value the caller supplied, so the on-disk index
for the column describes
+ // something else: for a placeholder column it indexes the placeholder.
Leaving the iterator
+ // unset makes the caller fall back to reading through this reader, the
same as the path that
+ // finds no reader at all. The base implementation would also run on
physical state this class
+ // never initializes.
+ Status new_index_iterator(const std::shared_ptr<IndexFileReader>&
/*index_file_reader*/,
+ const TabletIndex* /*index_meta*/, const
std::string& /*rowset_id*/,
+ uint32_t /*segment_id*/, size_t
/*rows_of_segment*/,
+ std::unique_ptr<IndexIterator>* /*iterator*/)
override {
+ return Status::OK();
+ }
+
Review Comment:
Removed the `ConstantColumnReader::new_index_iterator` override from this
PR, and this thread is the reason. Making the reader refuse index init based on
its dynamic type cannot be complete: cache eviction and a concurrent bare miss
(the two sibling P1s here) both rebuild a physical reader for the same column,
and the index path passes no constant, so the guard is bypassed. Residency and
arrival order can't carry a correctness property.
The reliable signal is the schema: the column's ordinal is commit-TSO /
version / binlog-TSO under known substitution conditions. #67995 makes
`Segment::new_index_iterator` (and the zone-map builders and the pushed-agg
gate) skip or synthesize based on that, which covers all three orderings at
once and fixes the pre-existing single-slot exposure too. Moving the whole
placeholder concern there rather than shipping a partial reader-type guard here.
This PR is now scoped to the cache contract only: a request carrying a
constant is not dropped on a hit, and the entry is replaced in place. That part
is complete and tested on its own.
--
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]
