LuciferYang opened a new pull request, #68018:
URL: https://github.com/apache/doris/pull/68018
### What problem does this PR solve?
Issue Number: close #67994
Problem Summary:
`__DORIS_COMMIT_TSO_COL__` stores a `0` placeholder on disk in a
single-version segment, and the real value reaches readers by building a
`ConstantColumnReader` instead of the on-disk one. That substitution goes
through the per-segment reader cache, whose lookup drops the requested constant:
```cpp
if (auto cached = _lookup({col_uid, {}})) {
*column_reader = cached;
return Status::OK();
}
```
So whichever caller populates the entry first decides what every later
caller gets. The expression zone-map builders request a reader without a
constant (`be/src/storage/segment/segment.cpp:129-130`,
`be/src/storage/segment/segment_iterator.cpp:3414-3415`) and the segment-level
one runs inside `Segment::new_iterator` (`:485-488`), before the
`SegmentIterator` and therefore before any `Segment::new_column_iterator` call
that would have installed the constant. Once the on-disk reader is cached,
`ConstantColumnReader::new_iterator` is never reached and reads of that column
return `0` as row data, not only as a zone map, until the entry or the segment
is evicted.
That the mechanism is real is already recorded in the tree:
`segment_zone_maps_can_answer_agg` walks every column with a bare request and
skips exactly one ordinal, with the reason spelled out
(`be/src/storage/segment/segment.cpp:148-160`): "Creating it here without one
would cache a reader that hands every later read the on-disk placeholder
instead."
This makes a request that carries a constant authoritative. On a hit, the
cached reader is returned only when the caller asked for no constant, or when
the cached reader is already a constant one; otherwise the constant reader is
built and replaces the entry, so later callers that cannot supply the constant
stop seeing the placeholder too. `ColumnReader::is_constant()` is the predicate
the cache needs to tell the two apart.
Replacing an entry required fixing the insert path. `_insert_locked_nocheck`
pushed a second LRU node for an existing key and overwrote only the map
iterator, leaving the first node reachable through the list but not the map;
eviction erases `_cache_map[tail->key]`, so when that stale node reached the
tail it erased the live entry of the same key while its node stayed in the
list. It now updates the existing node in place.
Nothing here changes what the zone-map builders themselves request. A
cache-level fix cannot repair an evaluation that already consumed the
placeholder summary, so the builders still have to ask for the constant; that
is #67995.
The concurrent case is deliberately left as last-writer-wins: two callers
that both miss can both insert, and if the bare one lands last the entry holds
the on-disk reader. The next request carrying a constant then falls through and
replaces it, and the row-read path always carries one, so the state self-heals
rather than needing a tie-break rule. I could not write a test that
distinguishes a tie-break rule from its absence, so I did not add one.
### Release note
Fixed a bug where a column whose stored value is a placeholder could be read
back as that placeholder after another caller warmed the segment's column
reader cache.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [X] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`ConstValueIsNotDroppedOnCacheHit` requests a reader without a constant,
then with one, and asserts the second is a constant reader whose segment zone
map is the degenerate non-null summary of that value; then that a following
bare request gets the same constant reader, that the cache still holds one
entry, and that asking again with a constant is a plain hit. Iterator output
has its own coverage in `constant_column_iterator_test.cpp`.
`SameKeyReplacementDoesNotLeaveAStaleLruNode` replaces uid 1's entry, fills
the cache past its capacity, and asserts uid 1 is gone from the reported
readers and that the count matches the capacity.
Both were checked by mutation: serving the cached reader unconditionally
makes the first test fail on `is_constant()`, and restoring the append-only
insert makes the second fail on both assertions (uid 1 still reported, four
entries instead of three).
### Behavior changed:
- [ ] No.
- [X] Yes.
A request carrying a constant now returns a constant reader even when the
column is already cached, and the cached entry is replaced.
### Does this need documentation?
- [X] No.
--
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]
