Copilot commented on code in PR #60079:
URL: https://github.com/apache/doris/pull/60079#discussion_r2711367308
##########
be/src/vec/exprs/vcompound_pred.h:
##########
@@ -347,6 +347,8 @@ class VCompoundPred : public VectorizedFnCall {
return Status::OK();
}
+ double execute_cost() const override { return 0.3; }
Review Comment:
`VCompoundPred::execute_cost()` returns a constant (0.3) and ignores its
child expressions’ costs. Since this cost is used for conjunct reordering,
dropping children costs can underestimate complex predicates (e.g., nested
function calls) and lead to suboptimal ordering. Please include the children’s
execute_cost in the returned value (or otherwise ensure the cost represents the
whole expression tree).
```suggestion
double execute_cost() const override {
double cost = 0.3;
for (const auto& child : _children) {
if (child) {
cost += child->execute_cost();
}
}
return cost;
}
```
##########
be/src/pipeline/exec/operator.cpp:
##########
@@ -199,19 +199,21 @@ Status OperatorXBase::init(const TPlanNode& tnode,
RuntimeState* /*state*/) {
_op_name = substr + "_OPERATOR";
if (tnode.__isset.vconjunct) {
- vectorized::VExprContextSPtr context;
- RETURN_IF_ERROR(vectorized::VExpr::create_expr_tree(tnode.vconjunct,
context));
- _conjuncts.emplace_back(context);
+ return Status::InternalError("vconjunct is not supported yet");
} else if (tnode.__isset.conjuncts) {
Review Comment:
This change makes `OperatorXBase::init()` fail whenever
`tnode.__isset.vconjunct` is true. Previously, `vconjunct` was converted into
an expr tree and added to `_conjuncts`; rejecting it here is a behavioral
regression that can break plan nodes emitted with `vconjunct` (even if
`vconjunct.nodes` is non-empty). Please restore `vconjunct` handling (or only
error if it’s truly unsupported and cannot appear), and keep behavior
consistent with the existing `conjuncts` path.
##########
be/src/vec/exprs/vexpr_context.cpp:
##########
@@ -473,5 +473,9 @@ uint64_t VExprContext::get_digest(uint64_t seed) const {
return _root->get_digest(seed);
}
+double VExprContext::execute_cost() const {
Review Comment:
`VExprContext::execute_cost()` unconditionally dereferences `_root`, but the
class already treats `_root == nullptr` as a valid state (e.g.,
`prepare_ann_range_search()` / `evaluate_ann_range_search()` early-return when
`_root` is null). This can crash when cost-based sorting is enabled and a
context has a null root. Please add a null check here and return an appropriate
base cost (e.g., 0.0) when `_root` is null.
```suggestion
double VExprContext::execute_cost() const {
if (_root == nullptr) {
// When there is no expression root, treat the cost as a base value.
// This avoids null dereferences while keeping a deterministic cost.
return 0.0;
}
```
--
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]
