Copilot commented on code in PR #63969:
URL: https://github.com/apache/doris/pull/63969#discussion_r3334388540
##########
be/src/runtime/runtime_predicate.cpp:
##########
@@ -59,6 +59,7 @@ Status RuntimePredicate::init_target(
int32_t target_node_id, phmap::flat_hash_map<int, SlotDescriptor*>
slot_id_to_slot_desc,
const int column_id) {
if (column_id < 0) {
+ _detected_target = true;
return Status::OK();
}
Review Comment:
`init_target()` writes `_detected_target` without holding `_rwlock` when
`column_id < 0`. Since `enable()/has_value()/get_value()` read these flags
under a shared lock, this introduces a data race and undefined behavior under
concurrent access.
##########
be/src/exec/operator/scan_operator.h:
##########
@@ -258,11 +258,6 @@ class ScanLocalState : public ScanLocalStateBase {
std::vector<int> get_topn_filter_source_node_ids(RuntimeState* state, bool
push_down) {
std::vector<int> result;
for (int id : _parent->cast<typename
Derived::Parent>()._topn_filter_source_node_ids) {
- if (!state->get_query_ctx()->has_runtime_predicate(id)) {
- // compatible with older versions fe
- continue;
- }
-
const auto& pred =
state->get_query_ctx()->get_runtime_predicate(id);
if (!pred.enable()) {
Review Comment:
`get_runtime_predicate(id)` requires `has_runtime_predicate(id)` (it DCHECKs
and then dereferences the map iterator). Calling it unconditionally here can
crash/UB if a plan contains `topn_filter_source_node_ids` but the FE did not
send `topn_filter_descs` (e.g. mixed-version/rolling upgrade or other plan
inconsistency).
##########
be/src/exec/operator/scan_operator.cpp:
##########
@@ -1250,11 +1250,6 @@ Status
ScanOperatorX<LocalStateType>::prepare(RuntimeState* state) {
_slot_id_to_slot_desc[slot->id()] = slot;
}
for (auto id : _topn_filter_source_node_ids) {
- if (!state->get_query_ctx()->has_runtime_predicate(id)) {
- // compatible with older versions fe
- continue;
- }
-
int cid = -1;
if
(state->get_query_ctx()->get_runtime_predicate(id).target_is_slot(node_id())) {
auto s = _slot_id_to_slot_desc[state->get_query_ctx()
Review Comment:
`QueryContext::get_runtime_predicate(id)` assumes
`has_runtime_predicate(id)` and will DCHECK + dereference an invalid iterator
if the runtime predicate descs are missing. After removing the guard, this loop
can now crash/UB for plans that carry `topn_filter_source_node_ids` without
corresponding `topn_filter_descs`.
##########
be/src/exec/operator/scan_operator.cpp:
##########
@@ -1250,11 +1250,6 @@ Status
ScanOperatorX<LocalStateType>::prepare(RuntimeState* state) {
_slot_id_to_slot_desc[slot->id()] = slot;
}
for (auto id : _topn_filter_source_node_ids) {
- if (!state->get_query_ctx()->has_runtime_predicate(id)) {
- // compatible with older versions fe
- continue;
- }
-
int cid = -1;
if
(state->get_query_ctx()->get_runtime_predicate(id).target_is_slot(node_id())) {
auto s = _slot_id_to_slot_desc[state->get_query_ctx()
Review Comment:
`QueryContext::get_runtime_predicate(id)` assumes
`has_runtime_predicate(id)` and will DCHECK + dereference an invalid iterator
if the runtime predicate descs are missing. After removing the guard, this loop
can now crash/UB for plans that carry `topn_filter_source_node_ids` without
corresponding `topn_filter_descs`.
##########
be/src/runtime/runtime_predicate.cpp:
##########
@@ -59,6 +59,7 @@ Status RuntimePredicate::init_target(
int32_t target_node_id, phmap::flat_hash_map<int, SlotDescriptor*>
slot_id_to_slot_desc,
const int column_id) {
if (column_id < 0) {
+ _detected_target = true;
return Status::OK();
}
Review Comment:
`init_target()` writes `_detected_target` without holding `_rwlock` when
`column_id < 0`. Since `enable()/has_value()/get_value()` read these flags
under a shared lock, this introduces a data race and undefined behavior under
concurrent access.
##########
be/src/exec/operator/scan_operator.h:
##########
@@ -258,11 +258,6 @@ class ScanLocalState : public ScanLocalStateBase {
std::vector<int> get_topn_filter_source_node_ids(RuntimeState* state, bool
push_down) {
std::vector<int> result;
for (int id : _parent->cast<typename
Derived::Parent>()._topn_filter_source_node_ids) {
- if (!state->get_query_ctx()->has_runtime_predicate(id)) {
- // compatible with older versions fe
- continue;
- }
-
const auto& pred =
state->get_query_ctx()->get_runtime_predicate(id);
if (!pred.enable()) {
Review Comment:
`get_runtime_predicate(id)` requires `has_runtime_predicate(id)` (it DCHECKs
and then dereferences the map iterator). Calling it unconditionally here can
crash/UB if a plan contains `topn_filter_source_node_ids` but the FE did not
send `topn_filter_descs` (e.g. mixed-version/rolling upgrade or other plan
inconsistency).
--
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]
