Repository navigation
[opt](point query) Reuse segments for point query row data lookup - #67649
Merged
Merged
Conversation
zyp-V
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
September 8, 2026 08:38
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: complete after two convergence rounds. I am requesting changes for two issues: retained-segment reuse drops the loader's unhealthy-object replacement behavior, and the default-enabled partial-row-store path still performs the second segment load/scan that this PR intends to remove.
Checkpoint conclusions:
- Goal, scope, and performance: the production change is focused and removes the second loader call for healthy full-row-store point queries. Partial
row_store_columnsqueries remain on the redundant path, so the stated optimization is incomplete there. - Correctness and conditions: ordinary healthy reads, key misses, delete filtering, row-cache hits, sequence handling, local/cloud identities, and non-contiguous physical segment IDs remain sound. The unhealthy-segment retry regression is blocking.
- Concurrency and lifecycle: no new thread or lock protocol is introduced. Shared rowset/segment ownership and reverse destruction order are safe across cache eviction and rowset retirement; mutable segment health is the separate boundary that must be restored.
- Compatibility, configuration, and persistence: the defaulted key-lookup output and retained legacy row-data overload preserve existing callers. There is no new FE/BE variable, wire/storage-format, transaction, or write-path change.
- Parallel paths and tests: the healthy full-row-store tests cover local/cloud IDs, eviction lifetime, cache hits, misses, deletion, references, output equality, and loader count. Missing coverage is partial row storage and unhealthy replacement, including their composition. Existing profile counters remain applicable, and the new sync-point hook is test-only in production builds.
- Validation status: per the review environment instructions, I did not build or run tests locally. At submission time the formatter/style checks are green, the external performance check is failing, and BE UT/compile are still pending.
- User focus: no additional focus was supplied, so the whole PR was reviewed.
zyp-V
force-pushed
the
reuse_segment_for_pointquery
branch
from
September 8, 2026 11:56
6f71042 to
6f6e1f8
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16768 ms |
Contributor
TPC-DS: Total hot run time: 81948 ms |
Contributor
ClickBench: Total hot run time: 14.82 s |
zyp-V
force-pushed
the
reuse_segment_for_pointquery
branch
from
September 9, 2026 07:51
6f6e1f8 to
cbe2155
Compare
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16901 ms |
Contributor
TPC-DS: Total hot run time: 82738 ms |
Contributor
ClickBench: Total hot run time: 14.91 s |
Member
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete after one convergence round at cbe215506c408a70f0e959b8d1ce61e597620d23. I am requesting changes because the two existing inline issues remain unresolved on this head; I am not duplicating those threads.
- P1 unhealthy replacement: the retained-segment overload bypasses
SegmentLoader::load_segment()'s completed-unhealthy check/replacement. A failure recorded by lazy column-metadata initialization between key lookup and row-data lookup is replayed by direct use, whereas the previous loader-backed path could replace the failed object. The later-race possibility mentioned in the reply does not remove this completed-before-check regression; the healthy hot path can remain direct while only non-healthy segments fall back to the loader. - P2 partial/no-row-store fallback: when
missing_col_uidsis nonempty, the executor still reacquires the rowset, callsload_segments, and scans by physical ID instead of using the acquired rowset and retained or health-replaced Segment. Besides leaving the claimed optimization incomplete, same-version rowset replacement can remove the old rowset from the active/stale maps even though this context still owns it.
Checkpoint conclusions:
- Goal, scope, and performance: the production change is focused and removes the second loader call for healthy full-row-store point queries. It does not complete that goal for partial
row_store_columnsor column-store-only fallback. - Correctness, conditions, and error handling: healthy reads, cache hits, key misses, delete filtering, sequence continuation, nullable output, Status propagation, local IDs, and noncontiguous cloud physical IDs are sound. The unresolved P1 changes recovery behavior for an already-failed cached Segment.
- Concurrency and lifecycle: no new thread or lock protocol is introduced. The retained
shared_ptr, paired rowset identity, cache-eviction lifetime, and reverse member destruction order are correct; mutable Segment health and the separate map reacquisition are the two unresolved boundaries above. - Compatibility, configuration, persistence, and writes: the trailing default output and legacy overload preserve current callers. There is no FE/BE variable, RPC/storage-format, dynamic configuration, transaction, persistence, or data-write change.
- Parallel paths and tests: current tests cover healthy full-row-store reuse, local/cloud-style IDs, same physical IDs across rowsets, eviction, reader references, row-cache hits, misses, deletion, output equality, and loader counts. They still need deterministic unhealthy-replacement and partial/no-row-store mixed-source coverage.
- Validation: review remained static as required by this runner.
build-support/check-build-hygiene.shpassed, and the live current-headCOMPILEandBE UTchecks are green. - Observability: existing point-query timers, read statistics, and slow-query logging remain applicable; no new metric is required for this internal handoff.
- User focus: no additional focus was supplied, so the complete PR was reviewed.
No new inline comments are included because every substantiated issue is already covered by the two live threads above.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Row-store point queries load rowset segments to locate a
primary key, then load and search those segments again to read the row data.
Return the matched segment from key lookup and retain it in the per-row read
context so row-data lookup can use it directly. Keep the existing lookup
interface for other callers and preserve the current branch's IOContext.
This removes the second segment-loader call and segment scan from the row
data stage; no latency or throughput benchmark was run.
Adapt the source change to BaseTablet and typed RowCursor fields. Port the
segment identity, loader-call count and row-data equality assertions, and
cover cache eviction and deleted keys as well.
Backported-from: 8c574d56ef1918693406bdea86c51eb26d657b3d
### Release note
Reduce repeated segment lookups in row-store point queries.
### Check List (For Author)
- Test: Manual review, clang-format 16, build hygiene and git diff --check passed.
- Unit Test: ASAN BE build and SegmentCacheTest attempted; compilation is
blocked by installed Thrift 0.16 lacking TOutput::instance(), while this
branch requires Thrift 0.24. No runtime tests completed.
- Static analysis: clang-tidy attempted; existing unmatched NOLINTEND in
be/src/core/types.h prevents a passing result.
- Behavior changed: No (query results and wire/storage formats are unchanged)
- Does this need documentation: No
zyp-V
force-pushed
the
reuse_segment_for_pointquery
branch
from
September 28, 2026 07:03
cbe2155 to
7d861bd
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28039 ms |
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
hello-stephen
pushed a commit
that referenced
this pull request
Oct 21, 2026
### What problem does this PR solve? Issue Number: None Related PR: #67649 When an automated review reconfirms an existing blocker whose heading starts with `**[P1] ...**` or `**[P1]** ...`, `submit_review.py` rejects the historical comment before posting the final review. This caused [run 39532829872](https://github.com/apache/doris/actions/runs/39532829872) and [run 39535990055](https://github.com/apache/doris/actions/runs/39535990055) to finish without delivering a review. Normalize the leading bold priority markup only when counting explicitly reconfirmed existing comments. New findings retain their strict format. Existing P0/P1 findings still request changes without duplicate inline comments, and existing P2/P3 findings remain invalid as blockers. ### Release note None ### Check List (For Author) - Test: - [x] Unit Test: `test_submit_review.py` — 40 passed; `test_run_review_with_resume.py` — 78 passed, 3 Linux-only tests skipped on macOS; `test_resolve_code_review_status.py` — 20 passed. - [x] Manual test: the new bold-comment cases fail against the original implementation. `git diff --check` passes. - Coverage includes P0/P1 counts, exact readback, repeated submission without reposting, rejection of P2/P3 and malformed or reply comments, and strict validation of new findings. GitHub API calls are mocked. - Behavior changed: - [x] Yes. Existing review comments with bold priority headings can be reconfirmed and delivered. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28698 ms |
Contributor
TPC-DS: Total hot run time: 152106 ms |
Contributor
ClickBench: Total hot run time: 24.5 s |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all seven changed files at c343cf2. Request changes because the existing P1 remains applicable; no new inline findings were identified.
Existing findings independently revalidated:
- P1: unhealthy-segment replacement remains unresolved. Key lookup initializes the primary-key index/Bloom filter without initializing column metadata. A concurrent reader can then store a transient footer/metadata failure in the retained segment's DorisCallOnce state. The new direct row-data path replays that failure; the previous loader path checks healthy_status and can replace the failed segment. The author's observation that failure can also occur after a health check does not eliminate recovery for a failure already present before that check. Carrying existing blocking comment 3956640174; no duplicate posted.
- P2: partial-row-store loading also remains applicable. Missing-column reads still load and search segments again. This is already reported and is not duplicated.
The reviewed flow is: request -> row-cache probe or key lookup -> retain matched segment and acquire rowset -> row-data iterator -> output -> release segment before rowset.
Critical checkpoint conclusions:
- Goal and proof: healthy full-row-store reads avoid the second loader call/segment search; new unit assertions check call counts and returned values. The existing P1/P2 limit correctness recovery and optimization coverage respectively. No latency or throughput measurement was supplied.
- Scope: focused additive API/context changes with a shared row-data helper and two test files.
- Concurrency and locks: per-request stages are sequential, while cached segments are shared with other readers. The health-state regression above remains; no new lock order or heavy work under a new lock was found.
- Lifecycle and static initialization: retained shared ownership survives cache eviction; rowset acquire/release and reverse member destruction preserve lifetime on success/error. Metadata accounting remains object-owned. No ownership cycle or new cross-TU initialization dependency was found.
- Configuration: no production configuration added; modified test flags and substituted singleton pointers are restored.
- Compatibility: legacy row-data overload and defaulted trailing key-lookup output preserve source callers. No wire/storage-format or dependency/libc-API changes; final ELF/glibc compatibility was not validated in this static review.
- Thrift/protobuf: unchanged; field-ID review is not applicable.
- Parallel paths: row-cache hits and legacy callers remain; partial/absent row-store reloads are the existing P2. Local contiguous and cloud noncontiguous physical segment IDs are covered by the added cases.
- Conditions and errors: optional segment selection and caller assertions are clear; Status propagation and IOContext/version/commit_tso setup remain shared. The omitted health gate is the existing P1.
- Coverage: inspected tests cover multiple rowsets/segments, repeated segment use, nulls, missing/deleted keys, eviction, reader-reference release, legacy/direct payload equality and cold/warm row-cache behavior. Unhealthy-transition and partial-row-store cases remain absent. Builds, tests and benchmarks were not run, as required by this review contract.
- Expected results: inserted values, nullable output and MySQL cell encodings match the assertions by inspection; no regression output files changed.
- Observability: existing statistics, timers and error reporting are reused; the added counting sync point is test/injection gated.
- Transactions and persistence: no persistence/replay/failover protocol changes; existing rowset and delete-bitmap selection are retained.
- Writes and crash cleanup: production changes are read-only; request-owned RAII cleanup introduces no new durable state.
- FE/BE variables: none added; no protocol send-path updates required.
- Performance: healthy row-store reads remove redundant loader traversal/search; partial-row-store repetition remains the existing P2. Segment ownership lasts for the request.
- Other checks: no new PCH/hub-header dependencies, explicit template families, source-including tests or hygiene-table changes; no further distinct issue substantiated.
User focus: no additional focus was supplied. Coverage is complete for the supplied diff and related read/lifetime/error paths through static inspection; runtime recovery, execution of added tests, final release ELF compatibility and measured performance remain unvalidated.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #67649 (comment)
Contributor
Author
@csun5285 这个 P1 对于点查来说我觉得是允许的， 而且目前这个改动路径只有点查会走到， 因为下一次 rpc 点查还会做健康检查的， 原来的写法只是防守 lookup key 的时候 segment 是好的， lookup data segment 不健康的情况， 所以中间加了一次 load segments 做健康检查； |
Contributor
|
run buildall |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Changes requested: the previously reported P1 health-replacement regression still applies to ec73ba4. No additional distinct findings were verified across the seven changed files; existing threads are not reposted.
Existing findings independently revalidated:
- P1: Preserve unhealthy-segment replacement before reuse. Key lookup loads the PK index/BF without necessarily initializing column metadata. Another scan can record a transient footer/metadata failure in the retained Segment's DorisCallOnce before this query's data stage. The new direct path replays that stored failure; the previous loader path would detect the already-unhealthy instance and replace it. The author's observation that a later race can still occur after a health check does not address this lost recovery case. This comment remains the existing blocker.
- P2: Reuse the retained segment in the missing-column path. Partial row-store queries still enter the following missing-column branch, reload rowset segments, and scan for the same physical ID. Both new fixtures use full-row storage, so their loader-count assertions do not cover this branch. This remains covered by the existing thread.
Invocation and ownership reviewed: RPC worker -> executor init -> sequential key lookup -> retained location/Segment/acquired rowset -> row-store and optional missing-column reads -> result serialization -> context teardown. Other queries can concurrently use the shared cached Segment.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Healthy full-row reads avoid the second loader call/scan. Added unit assertions cover this and output values; E1/E2 prevent full goal satisfaction. Tests were inspected, not run. |
| Scope and reuse | Production changes are focused; old row-data overload and shared iterator initialization are retained. |
| Concurrency and locks | Per-executor stages are sequential; shared Segment metadata initialization can race with another scan (E1). Rowset snapshot capture remains under the header lock, with IO outside it. No new locks, nesting, threads, or deadlock mechanism found. |
| Lifecycle and memory | The context releases its Segment before its acquired rowset. Shared rowset ownership prevents local GC deletion. SegmentCacheHandle already retains shared_ptrs after releasing LRU handles; the new owner follows that model. Metadata accounting and weak, identity-checked cache callbacks remain intact. No ownership cycle or new static-initialization dependency found. |
| Configuration | No configuration item added or changed; dynamic-setting behavior is unchanged. |
| Compatibility | The original row-data entry point remains; the key-lookup output is a defaulted trailing argument. No storage/wire format or rolling-upgrade change. No new libc API identified; final ELF compatibility was not audited. |
| Thrift/protobuf | No generated definition changes. |
| Parallel paths | Local contiguous and cloud explicit/legacy segment IDs retain correct physical identity; vector positions are used only to select the matched object. Mixed/column-store repeated loading is E2. Existing write and rowid-fetcher callers retain prior behavior. |
| Conditional checks | Optional output/direct-path checks match the API contract; segment output is set only after key/delete-bitmap acceptance. No additional conditional-path defect found. |
| Test coverage | New tests cover missing/deleted keys, NULL values, multiple rowsets/segments, offset row IDs, local/noncontiguous cloud layouts, eviction/lifetime, legacy equality, loader counts and row-cache results. Unhealthy replacement and mixed-source cases remain absent as already covered by E1/E2. Fixture setup, writer use, global cache restoration and teardown were checked statically. |
| Test results | No golden-result files changed. Expected row values and serialized NULL bytes are consistent with the inspected code; execution remains unverified. |
| Observability | Existing timers, stats, logs and IOContext propagation remain. Test instrumentation is scoped and disabled in ordinary builds; no additional observability defect found. |
| Transaction/persistence/failover | Production changes add no writes or persistent metadata; rowset version/commit context and delete-bitmap acceptance remain on the same path. No new failover protocol. |
| Write atomicity/crash behavior | No production write or atomicity change. Normal/error ownership cleanup and GC protection retain their existing mechanisms. |
| FE-BE variables | No new transmitted variable or sending path. |
| Performance | The healthy full-row fast path removes intended redundant work; partial-row-store work remains E2. No latency/throughput benchmark was run or improvement amount inferred. |
| Other correctness/errors | Same located rowset/physical segment and row ID are retained; iterator/read Status and RPC exception conversion remain. No additional distinct wrong-result or error-handling issue verified beyond E1. |
No additional user focus was supplied. Static review covers the entire supplied diff and relevant upstream/downstream paths; there are no unresolved static candidates. No builds, tests, formatting/static-analysis tools, benchmarks, remote cloud runs, or release-artifact ABI audits were performed under the review-only contract. Cloud tests model segment-list layout using local files; runtime race/recovery and mixed-source coverage limitations are stated above.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #67649 (comment)
Contributor
TPC-H: Total hot run time: 28468 ms |
Contributor
TPC-DS: Total hot run time: 150155 ms |
Contributor
ClickBench: Total hot run time: 24.26 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28488 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 150870 ms |
Contributor
ClickBench: Total hot run time: 24.4 s |
yiguolei
approved these changes
Oct 24, 2026
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Changes requested because existing P1 comment 3956640174 still applies to head 7d3d347. No distinct new inline findings were verified; the existing threads are not reposted.
The P1 remains a concrete failure-recovery regression: concurrent RPC workers can share Segment S; after key lookup, another reader can cache a transient column-metadata initialization failure in S. If that failure finishes and the IO fault clears before row-data lookup, the legacy loader detects unhealthy S and opens a replacement, while the new direct path replays S's stored failure. The author's observation about a residual race after the old health check does not invalidate this schedule. Reuse should preserve replacement of an already-unhealthy instance; failure frequency was not measured.
Existing P2 comment 3956640179 also remains applicable: with row_store_columns='k,v1', selecting k,v2 reaches the default-enabled column-store fallback, which still reloads and scans for the retained segment. This is an incomplete optimization in a pre-existing branch; no new mixed-source result corruption was established.
Individual critical checkpoint conclusions:
- Goal and proof: healthy full-row-store point reads reuse the exact matched segment and remove the second load_segments call. Added unit assertions cover this behavior; P1/P2 qualify completion of the intended behavior.
- Scope and clarity: changes are focused on optional segment output, per-row ownership, a shared row-data helper, and test instrumentation.
- Concurrency and locks: separate light-pool RPC workers share cached segments and their lazy metadata state. Request contexts remain local; the existing shared tablet-header lock protects rowset selection, IO remains outside it, and no new lock-order edge was found. Shared unhealthy transitions remain P1.
- Lifecycle and memory: the retained shared Segment survives key-cache destruction and eviction. Its acquired Rowset remains owned, and member destruction releases Segment before Rowset; query-error cleanup uses the existing memory tracker. No new cycle or static-initialization dependency was found.
- Configuration: no configuration item was added or changed; tests restore the globals they modify.
- Compatibility: the legacy row-data overload remains and existing key-lookup callers use the default-null new output. These are internal BE interfaces; RPC/storage formats and libc/dependency usage are unchanged.
- Thrift/protobuf: no definitions changed; cross-release field-ID checks are inapplicable.
- Parallel paths: full/partial/absent row store, row-cache hit/miss, local/cloud IDs, MoW key probes and rowid-fetch callers were traced. Legacy paths retain loader behavior; partial/column-only fallback remains the known incomplete optimization.
- Conditions and errors: segment output is assigned after the winning key passes bitmap checks. Misses and failed lookups do not enter direct row-data access; existing Status propagation and current IOContext are preserved.
- Test coverage: new cases cover multiple rowsets/segments, shared segments, noncontiguous cloud IDs, NULL, missing/deleted keys, eviction/lifetime, legacy byte parity, complete output and warm row-cache bypass. Unhealthy-transition replacement and partial-row-store mixed-source coverage remain missing as already requested in the existing threads.
- Test results: no golden/.out files changed. Expected row values, nullable output and MySQL text bytes match the test input by static inspection; no test execution is claimed.
- Observability: existing point-query timings, cache/row hits and IO statistics remain available. The added SyncPoint callback is test/injection instrumentation and compiles away in ordinary production builds; no additional metric requirement was identified.
- Transactions, persistence and failover: no durable metadata, EditLog, publish protocol or failover handling changed. Version selection and delete-bitmap checks precede returning the retained segment as before.
- Data writes and crashes: no production data-write behavior changed. Shared ownership protects the selected rowset during compaction/GC; no distinct new leak, deadlock or cleanup failure was found.
- FE/BE values: no transmitted variable was added. Existing iterator initialization still receives rowset version/commit_tso and the current request's IOContext.
- Performance: healthy full-row-store reads avoid repeated loader/cache traversal and segment scanning. Retention adds one shared pointer per found key; it ends with the executor. P2 remains; no latency or throughput improvement was measured.
- Other correctness: physical segment identity is preserved independently of vector position and rowset identity. Visibility/bitmap decisions, JSONB decoding, nullable values, cache bypass and legacy dispatch were checked without a distinct additional defect. No new hub-header include, source-.cpp test inclusion or template-pairing change was introduced.
All seven changed files and assigned upstream/downstream mechanisms were reviewed. No additional user focus was supplied. Three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS; every risk was independently confirmed as already covered or dismissed with evidence. Review converged in one round.
Static inspection only: no builds, unit/regression tests or benchmarks were run. The cloud-ID unit cases use local files and do not prove live remote-cloud integration. There are no unresolved new candidates or unreviewed changed-file areas within this static review scope.
Review converged after 1 round(s).
Existing P0/P1 findings confirmed for this head: #67649 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Row-store point queries load rowset segments to locate a primary key, then load and search those segments again to read the row data. Return the matched segment from key lookup and retain it in the per-row read context so row-data lookup can use it directly. Keep the existing lookup interface for other callers and preserve the current branch's IOContext. This removes the second segment-loader call and segment scan from the row data stage; no latency or throughput benchmark was run.
Release note
Reduce repeated segment lookups in row-store point queries.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)