Copilot commented on code in PR #60079:
URL: https://github.com/apache/doris/pull/60079#discussion_r2708545340
##########
be/src/vec/functions/function.h:
##########
@@ -170,6 +170,8 @@ class IFunctionBase {
virtual const DataTypes& get_argument_types() const = 0;
virtual const DataTypePtr& get_return_type() const = 0;
+ virtual double execute_cost() const { return 1.0; }
+
Review Comment:
Adding execute_cost() to IFunctionBase is fine, but note that most functions
appear to be wrapped by DefaultFunction (be/src/vec/functions/function.h:436+).
Since DefaultFunction doesn’t override/forward execute_cost(), any per-function
overrides (e.g. in IFunction implementations) won’t be observed via
FunctionBasePtr. Consider forwarding execute_cost() in DefaultFunction (or
otherwise ensuring the wrapper preserves the underlying function’s cost).
##########
be/src/vec/exec/format/parquet/vparquet_group_reader.cpp:
##########
@@ -188,6 +188,12 @@ Status RowGroupReader::init(
_lazy_read_ctx.missing_columns_conjuncts.end());
RETURN_IF_ERROR(_rewrite_dict_predicates());
}
+ if (_state->query_options().enable_adjust_conjunct_order_by_cost) {
+ std::ranges::sort(_filter_conjuncts,
Review Comment:
Minor style: please keep spacing consistent with the rest of the file (`if
(`).
##########
be/src/vec/exprs/vectorized_fn_call.cpp:
##########
@@ -640,5 +640,9 @@ Status VectorizedFnCall::evaluate_ann_range_search(
return Status::OK();
}
+double VectorizedFnCall::execute_cost() const {
+ return _function->execute_cost();
Review Comment:
VectorizedFnCall::execute_cost() currently returns only
_function->execute_cost(), ignoring the cost of evaluating child expressions.
This will underestimate cost for nested expressions (e.g. eq(add(a,b),c)) and
can lead to suboptimal conjunct ordering. Consider combining the function cost
with children costs (e.g. include VExpr::execute_cost() or sum children here).
```suggestion
double cost = 0.0;
if (_function != nullptr) {
cost += _function->execute_cost();
}
for (const auto& child : _children) {
if (child != nullptr) {
cost += child->execute_cost();
}
}
return cost;
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3087,6 +3087,9 @@ public void setDetailShapePlanNodes(String
detailShapePlanNodes) {
})
public boolean enablePhraseQuerySequentialOpt = true;
+ @VariableMgr.VarAttr(name = "enable_adjust_conjunct_order_by_cost",
needForward = true)
+ public boolean enableAdjustConjunctOrderByCost = true;
Review Comment:
This session variable defaults to true, meaning predicate reordering by cost
will be enabled by default. Since this can change evaluation order (and
potentially error/short-circuit behavior), consider defaulting it to false
(similar to other cost-based reordering toggles like
enableJoinReorderBasedCost).
```suggestion
public boolean enableAdjustConjunctOrderByCost = false;
```
--
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]
