[fix](be) Keep a requested constant value on a column reader cache hit - #68018
LuciferYang wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: changes requested (static review of head 2e7d6a02e4c7b78403c1304856e7ab0aa4c0df71).
One blocking correctness issue is reported inline: the new physical-to-constant cache upgrade can feed ConstantColumnReader into eager physical inverted-index initialization and turn a previously successful hidden commit-TSO query into an initialization error.
Critical checkpoints:
- Goal, scope, and correctness: the focused cache suitability check fixes the reported physical-reader hit for ordinary row/segment-zone-map reads, and the same-key upsert fixes duplicate LRU nodes. It is not correct for the indexed continuation described inline.
- Concurrency and lifecycle: cache map/list mutation is mutex-protected;
splicepreserves the stored iterator; returnedshared_ptrs remain valid across replacement/eviction. Last-writer-wins construction creates no second distinct defect. - Configuration, compatibility, persistence, and writes: no new configuration, FE/BE field, RPC, serialized format, transaction/write/recovery behavior, or rolling-upgrade state is introduced.
- Parallel and special paths: constant data and segment-zone-map consumers are correct; page-zone-map, bloom, and later predicate-pruning paths are conservative. The eager inverted-index consumer is the unsafe exception. The separately tracked placeholder zone-map-builder gap in #67995 was not duplicated.
- Tests and results: the added unit tests distinguish the direct O-to-C cache fix and stale-LRU-node fix, but do not cover the physical-warm plus hidden-index continuation that exposes the inline issue. Per the review contract, no local build or tests were run. At this reviewed head, GitHub reports BE UT, COMPILE, P0, NonConcurrent, External Regression, Cloud regression, coverage, formatter, and style checks passed; performance remains pending. These are CI claims, not independent execution.
- Observability and performance: cache counters remain internally consistent with resident-key lookup semantics, and the upsert remains O(1); no additional substantive metric or hot-path regression was found.
- User focus: no additional review focus was supplied.
Review completion: complete after three bounded rounds. All candidates were independently verified and deduplicated; the accepted set is the single inline issue below, with no unresolved or capped finding.
| // 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())) { |
There was a problem hiding this comment.
[P1] Keep constant readers out of physical index initialization. With an inverted index on __DORIS_COMMIT_TSO_COL__, an OR such as tso < 10 OR tso > 20 remains a common expression, so segment expression-zone-map setup first caches the physical reader. This condition then replaces it with ConstantColumnReader during column-iterator setup; the following bare, eager index lookup receives that constant and calls non-virtual ColumnReader::new_index_iterator even though its physical type/index state was never initialized, so the query fails. This is new: for a real TSO greater than 20, both placeholder 0 and the real value satisfy the OR, so the pre-PR physical-warmed path returned the correct projected rows. Please skip physical index creation for constant readers or separate the cache entries, and cover this ordering in a test.
TPC-H: Total hot run time: 16974 ms |
TPC-DS: Total hot run time: 82134 ms |
ClickBench: Total hot run time: 14.7 s |
|
Fixed, with one correction to the framing. The mechanism is real. It is not new to this PR, though. The predicate loop in I also could not confirm the premise that such an index can exist: The fix is on the reader rather than at the call site: The full ordering test you asked for needs a segment carrying an inverted index on that hidden column, which I could not build. What I have instead is the reader refusing regardless of how it reached index setup.
|
|
run buildall |
|
/review |
|
run beut |
|
run cloud_p0 |
|
run performance |
There was a problem hiding this comment.
Automated review result: changes requested (static review of exact head 3c7b4c426417d2f96beab15082a670fd7fc54d1b).
One blocking correctness issue is reported inline: the new same-key upsert allows an in-flight physical-reader miss to overwrite a completed constant-reader upgrade, so placeholder index or zone-map state can still drive filtering.
Critical checkpoints:
- Goal and proof: the sequential physical-to-constant cache upgrade and constant-reader index dispatch are fixed and unit-covered, but the stated authoritative-constant behavior is not preserved under concurrent misses.
- Scope: the four-file change is focused on reader polymorphism, cache replacement, and targeted BE unit tests.
- Concurrency: map/list operations are mutex-protected, but lookup, construction, and upsert form a semantic lost-update race; this is the blocking P1. No additional lock-order or deadlock issue was found.
- Lifecycle/static initialization: the non-null constant is the immutable commit TSO for one rowset/segment identity; reader/iterator ownership remains safe, with no cycle or cross-TU initialization issue.
- Configuration: no configuration item is added or changed.
- Compatibility/rolling upgrade: no wire, storage-format, RPC, or persisted-state change is introduced; the virtual dispatch change is internal to BE.
- Parallel paths: stable fresh-null inverted/ANN fallback retains row evaluation correctly. Bare index and expression-zone-map consumers must observe the authoritative constant, which the accepted race can violate. The existing sequential index-dispatch thread is not duplicated.
- Conditional checks: the new
is_constant()condition explains and handles the sequential cases, but it does not define the concurrent winner at insertion. - Test coverage: sequential replacement, one-node LRU accounting, constant zone-map/type behavior, and direct index no-op are covered. A barrier-controlled mixed physical/constant miss test and authoritative-winner return assertion are missing.
- Test results: no builds or tests were run locally because the review contract requires static-only review; no
.outfiles apply. - Observability: cache gauges remain balanced; no additional required log or metric was identified.
- Transaction/persistence: no EditLog, transaction, publish, or recovery data is changed.
- Data writes/crash safety: no write path or on-disk mutation is modified.
- Cross-layer variables: no new FE-BE variable or protocol field is introduced.
- Performance/memory: replacement remains O(1), avoids duplicate LRU nodes, and adds no row-loop work or ownership leak.
- Other issues: different-constant reuse and reused non-null index-output hypotheses were dismissed with call-path evidence; the separate bare placeholder zone-map issue is already tracked/fenced and was not duplicated.
User focus: no additional review focus was supplied.
Review completion: complete after two bounded rounds. The second normal and risk-focused reviews both returned NO_NEW_VALUABLE_FINDINGS; all candidates are accepted, dismissed with evidence, or duplicate-fenced. Accepted set: the single inline P1 below.
| // 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()) { |
There was a problem hiding this comment.
[P1] Preserve the constant reader across concurrent misses. _lookup releases _cache_mutex before construction, so a bare request can miss and start building the physical reader, a constant request can then insert and return its reader, and the first request reaches this branch last and overwrites it. Since a cached Segment is shared, a query that already created a ConstantColumnIterator can then do the bare index lookup and receive the placeholder's physical index; for example, real commit TSO 42 with tso > 20 can be eliminated by an index containing 0, with the predicate removed from row fallback. This is the inverse concurrent order from the existing sequential comment. Please make the compare-and-upsert constant-dominant regardless of arrival order, return the selected authoritative reader to the caller, and add a barrier-controlled mixed physical/constant miss test.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 16923 ms |
TPC-DS: Total hot run time: 83555 ms |
ClickBench: Total hot run time: 14.75 s |
2 participants
What problem does this PR solve?
Issue Number: close #67994
Problem Summary:
__DORIS_COMMIT_TSO_COL__stores a0placeholder on disk in a single-version segment, and the real value reaches readers by building aConstantColumnReaderinstead of the on-disk one. That substitution goes through the per-segment reader cache, whose lookup drops the requested constant: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 insideSegment::new_iterator(:485-488), before theSegmentIteratorand therefore before anySegment::new_column_iteratorcall that would have installed the constant. Once the on-disk reader is cached,ConstantColumnReader::new_iteratoris never reached and reads of that column return0as 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_aggwalks 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_nocheckpushed 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.
One more change came out of review.
Segment::new_index_iteratorfetches the reader without a constant (be/src/storage/segment/segment.cpp:1069) and then callsreader->new_index_iterator(:1108), which was not virtual, so a constant reader ran the base implementation. That branches on the raw_type/_meta_typemembers, whichConstantColumnReadernever sets, and ends inINVERTED_INDEX_NOT_SUPPORTED: a query on a column with an index would fail rather than return rows.ConstantColumnReadernow overrides it and leaves the iterator unset, the same as the path that finds no reader at all, so the caller falls back to reading through this reader. That is the right answer independently of this bug, because the on-disk index for a placeholder column indexes the placeholder.That failure does not need this PR: the predicate loop in
Segment::new_iterator(:422-437) already installs the constant reader beforeSegmentIteratorcreates index iterators (be/src/storage/segment/segment_iterator.cpp:1853,:1885), so an indexed column reached this today. This PR adds one more ordering, where the expression zone-map builder warms the entry physically first.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)
ConstValueIsNotDroppedOnCacheHitrequests 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 inconstant_column_iterator_test.cpp.SameKeyReplacementDoesNotLeaveAStaleLruNodereplaces 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).NewIndexIteratorIsANoOpasserts that a constant reader returns OK and no iterator. Also mutation-checked: delegating the override to the base implementation makes it fail with[E-6002] Failed to load inverted index: index metadata is null.The ordering the reviewer asked to cover, physical warm plus a hidden indexed column, needs a segment fixture carrying an inverted index on
__DORIS_COMMIT_TSO_COL__, which I could not build; the override makes the reader itself refuse regardless of how it got there, and that part is covered.Behavior changed:
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?