github-actions[bot] commented on code in PR #67915:
URL: https://github.com/apache/doris/pull/67915#discussion_r4003693764
##########
be/src/exec/operator/olap_scan_operator.cpp:
##########
@@ -635,6 +646,21 @@ bool OlapScanLocalState::_is_binlog_merge_scan() const {
return scan_type == TBinlogScanType::MIN_DELTA || scan_type ==
TBinlogScanType::DETAIL;
}
+// Give each segment a scanner of its own for queries like:
+// SELECT k1 FROM t ORDER BY l2_distance_approximate(embedding, [1.0, 2.0])
LIMIT 2
+// SELECT k1 FROM t WHERE msg MATCH_PHRASE 'error timeout'
+bool OlapScanLocalState::_use_scan_parallelism_by_per_segment() {
+ // TODO: Use optimize_index_scan_parallelism for ann range search in the
future.
+ // Currently, ann topn is enough
+ if (state()->query_options().__isset.optimize_index_scan_parallelism &&
+ state()->query_options().optimize_index_scan_parallelism &&
_ann_topn_runtime != nullptr) {
+ return true;
+ }
+ return config::is_cloud_mode() &&
Review Comment:
[P1] Preserve global BM25 statistics before splitting by segment. This
branch also covers score-enabled MATCH/SEARCH scans. Each generated scanner
gets one segment from one rowset, and `OlapScanner::_prepare_impl()` builds a
fresh `CollectionStatistics` from only that scanner's `rs_splits`; those
rowset-local document-frequency and length values feed BM25. Rows from
different rowsets are therefore ranked with incompatible IDF/avgdl values, so a
final `ORDER BY score() LIMIT ...` can return the wrong row. Also, collection
walks every segment of the supplied rowset, so an N-segment rowset now repeats
that work N times. Compute/share statistics from the original complete read
source, or keep score queries on the prior strategy, and add a cloud
multi-rowset TopN regression.
##########
be/src/exec/operator/olap_scan_operator.cpp:
##########
@@ -460,6 +460,17 @@ static bool contains_expr_node_type(const VExprSPtr& expr,
TExprNodeType::type n
});
}
+// Find MATCH recursively; ones nested in AND / OR / NOT count too.
+static bool is_match_expr(const VExprSPtr& expr) {
+ DORIS_CHECK(expr != nullptr);
+ if (expr->node_type() == TExprNodeType::MATCH_PRED ||
Review Comment:
[P1] Gate this on index execution, not only on expression shape. With
`enable_inverted_index_query=false` (or a MATCH column without an iterator),
this still returns true even though SegmentIterator skips index evaluation and
runs the supported row fallback. On a single large segment, the old row-count
builder can split that expensive fallback across scanners, while this route
creates exactly one scanner and serializes it; on many segments it also
triggers the unbounded population noted separately. Row-only wrappers have the
same false positive because their roots never dispatch
`evaluate_inverted_index()` to the MATCH child. Require index execution to be
enabled and a usable/index-evaluable root before selecting this strategy.
##########
be/src/exec/operator/olap_scan_operator.cpp:
##########
@@ -764,15 +790,7 @@ Status
OlapScanLocalState::_init_scanners(std::list<ScannerSPtr>* scanners) {
std::max<int64_t>(1024,
state()->parallel_scan_min_rows_per_scanner());
scanner_builder.set_max_scanners_count(max_scanners_count);
scanner_builder.set_min_rows_per_scanner(min_rows_per_scanner);
- // If the session variable is set, force one scanner per segment.
- if (state()->query_options().__isset.optimize_index_scan_parallelism &&
- state()->query_options().optimize_index_scan_parallelism) {
- // TODO: Use optimize_index_scan_parallelism for ann range search
in the future.
- // Currently, ann topn is enough
- if (_ann_topn_runtime != nullptr) {
- scanner_builder.set_scan_parallelism_by_per_segment(true);
- }
- }
+
scanner_builder.set_scan_parallelism_by_per_segment(_use_scan_parallelism_by_per_segment());
Review Comment:
[P1] Keep segment-aligned scans within the configured scanner cap.
`_build_scanners_by_per_segment()` creates one scanner for every segment and
never consults `parallel_scan_max_scanners_count` or the minimum-rows setting;
initialization then creates a delegate and pending task for every scanner
before execution concurrency is applied. A cloud table with many historical
segments can therefore allocate an unbounded scanner/task population even with
the cap set to 1, and LIMIT/cancellation only clean it up afterward. Group
whole segments into at most the configured number of scanner read sources so no
segment is split while scanner cardinality remains bounded.
##########
be/src/exec/operator/olap_scan_operator.cpp:
##########
@@ -635,6 +646,21 @@ bool OlapScanLocalState::_is_binlog_merge_scan() const {
return scan_type == TBinlogScanType::MIN_DELTA || scan_type ==
TBinlogScanType::DETAIL;
}
+// Give each segment a scanner of its own for queries like:
+// SELECT k1 FROM t ORDER BY l2_distance_approximate(embedding, [1.0, 2.0])
LIMIT 2
+// SELECT k1 FROM t WHERE msg MATCH_PHRASE 'error timeout'
+bool OlapScanLocalState::_use_scan_parallelism_by_per_segment() {
+ // TODO: Use optimize_index_scan_parallelism for ann range search in the
future.
+ // Currently, ann topn is enough
+ if (state()->query_options().__isset.optimize_index_scan_parallelism &&
+ state()->query_options().optimize_index_scan_parallelism &&
_ann_topn_runtime != nullptr) {
+ return true;
+ }
+ return config::is_cloud_mode() &&
+ std::ranges::any_of(_common_expr_ctxs_push_down,
Review Comment:
[P2] Include pushed-down MATCH virtual-column projections in this decision.
Projection-only MATCH is stored in `_slot_id_to_virtual_column_expr`, not
`_common_expr_ctxs_push_down`, yet every scanner clones that context and
SegmentIterator evaluates it over `num_rows()` for the whole segment before
materializing the selected row range. A large segment split among N scanners
therefore still repeats the same index evaluation N times, so this misses a
supported path with the exact cost the PR targets. Inspect the virtual-column
roots here as well (including any `VirtualSlotRef` unwrapping) and add a cloud
projection regression.
--
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]
