github-actions[bot] commented on code in PR #67649:
URL: https://github.com/apache/doris/pull/67649#discussion_r3956640179
##########
be/src/service/point_query_executor.cpp:
##########
@@ -554,8 +554,8 @@ Status PointQueryExecutor::_lookup_row_data() {
io_ctx.remote_scan_cache_write_limiter =
_remote_scan_cache_write_limiter.get();
RETURN_IF_ERROR(_tablet->lookup_row_data(
_row_read_ctxs[i]._primary_key,
_row_read_ctxs[i]._row_location.value(),
- *(_row_read_ctxs[i]._rowset_ptr),
_profile_metrics.read_stats, value,
- use_row_cache, &io_ctx));
+ _row_read_ctxs[i]._segment,
*(_row_read_ctxs[i]._rowset_ptr),
Review Comment:
**[P2] Reuse the retained segment in the missing-column path**
With partial `row_store_columns` (and the default-enabled column-store
fallback), `missing_col_uids` remains non-empty, so the code below still
reacquires the rowset, calls `load_segments`, and scans for the same physical
segment ID even though this context now owns the exact matched segment. The new
call-count test uses `store_row_column=true`, so it never enters this branch.
Please route this read through the retained or health-replaced segment,
preserving the unhealthy-segment fallback, and add a partial-row-store case
that proves mixed-source results and no second healthy-path load.
##########
be/src/storage/tablet/base_tablet.cpp:
##########
@@ -422,6 +429,26 @@ Status BaseTablet::lookup_row_data(const Slice&
encoded_key, const RowLocation&
RowsetSharedPtr input_rowset,
OlapReaderStatistics& stats,
std::string& values, bool write_to_cache,
const io::IOContext* io_ctx) {
+ return _lookup_row_data(encoded_key, row_location, nullptr,
std::move(input_rowset), stats,
+ values, write_to_cache, io_ctx);
+}
+
+Status BaseTablet::lookup_row_data(const Slice& encoded_key, const
RowLocation& row_location,
+ const segment_v2::SegmentSharedPtr& segment,
+ RowsetSharedPtr input_rowset,
OlapReaderStatistics& stats,
+ std::string& values, bool write_to_cache,
+ const io::IOContext* io_ctx) {
+ DCHECK(segment != nullptr);
+ DCHECK_EQ(segment->id(), row_location.segment_id);
+ return _lookup_row_data(encoded_key, row_location, &segment,
std::move(input_rowset), stats,
Review Comment:
**[P1] Preserve unhealthy-segment replacement before reuse**
This direct overload skips `SegmentLoader::load_segment()`'s
`healthy_status()` gate. For example, key lookup can retain S before column
metadata is initialized; if another reader stores a transient metadata failure
in S's once-state, the legacy row-data path re-entered the loader and replaced
S, whereas this path calls `new_column_iterator` on S and deterministically
replays the failure. Please reuse only while the retained segment is healthy,
fall back to the loader-backed path otherwise, and ensure any replacement is
also used by the later missing-column read. Add an unhealthy-transition test
that proves replacement rather than merely observing the failure.
--
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]
