github-actions[bot] commented on code in PR #64563:
URL: https://github.com/apache/doris/pull/64563#discussion_r3488078554
##########
be/src/exprs/lambda_function/varray_map_function.cpp:
##########
@@ -329,52 +363,105 @@ class ArrayMapFunction : public LambdaFunction {
}
private:
- bool _contains_column_id(const std::vector<int>& output_slot_ref_indexs,
int id) const {
- const auto it = std::find(output_slot_ref_indexs.begin(),
output_slot_ref_indexs.end(), id);
- return it != output_slot_ref_indexs.end();
+ struct LambdaArgumentBinding {
+ bool bind_by_name = true;
+ size_t argument_size = 0;
+ std::vector<std::string> names;
+ };
+
+ Status _prepare_lambda_argument_binding(const VExprSPtr& expr, size_t
expected_argument_size,
+ LambdaArgumentBinding&
argument_binding) const {
+ DORIS_CHECK_EQ(expr->node_type(), TExprNodeType::LAMBDA_FUNCTION_EXPR);
+ const auto* lambda_expr = assert_cast<const
VLambdaFunctionExpr*>(expr.get());
+
+ argument_binding.argument_size = 0;
+ argument_binding.names.clear();
+ argument_binding.bind_by_name = lambda_expr->has_argument_names();
+
+ if (!argument_binding.bind_by_name) {
Review Comment:
This makes the new optional thrift field effectively mandatory for nested
`array_map` on the BE side. During a mixed-version rollout an old FE still
serializes `LAMBDA_FUNCTION_EXPR` without `lambda_argument_names`; with this
branch the new BE returns `InternalError` for any nested lambda body before
checking whether the nested lambda actually captures an outer argument. That
breaks previously valid local nested-lambda shapes such as `array_map(x ->
array_map(y -> y + 1, x), nested_arr)` while only BEs are upgraded. Please keep
a compatible legacy fallback for nested plans that can still be resolved by
column ids, or gate the new behavior on an FE/BE protocol/version check instead
of failing solely because the optional field is absent.
##########
be/src/exprs/lambda_function/varray_sort_function.cpp:
##########
@@ -221,6 +250,146 @@ class ArraySortFunction : public LambdaFunction {
return Status::OK();
}
+private:
+ Status _set_comparator_argument_gap(const VExprSPtr& expr,
+ const std::vector<std::string>*
argument_names) const {
+ if (expr->is_column_ref()) {
+ auto* ref = static_cast<VColumnRef*>(expr.get());
+ RETURN_IF_ERROR(_validate_comparator_argument_ref(*ref,
argument_names));
+ ref->set_gap(0);
+ return Status::OK();
+ }
+
+ if (expr->is_slot_ref() || expr->is_virtual_slot_ref()) {
+ return Status::NotSupported(
+ "array_sort comparator only supports its own lambda
arguments, but found "
+ "captured slot ref '{}'",
+ expr->expr_name());
+ }
+
+ if (_is_lambda_call_with_lambda_expr(expr)) {
+ // array_sort comparator arguments live in a position-based,
comparator-local frame
+ // that is invisible to nested lambda frames. Reject unsupported
nested captures during
+ // prepare, otherwise execution would later fail with an internal
missing-column error.
+ // For example, array_sort((x, y) -> array_map(z -> z + x,
nested_arr), arr) is
+ // rejected because the inner array_map lambda captures the
comparator-local x; while
+ // array_sort((x, y) -> array_map(x -> x + 1, nested_arr), arr) is
still valid because
+ // the inner x is array_map's own argument and shadows the
comparator argument.
+
RETURN_IF_ERROR(_reject_nested_lambda_capture_of_comparator_argument(
+ assert_cast<const
VLambdaFunctionExpr*>(expr->children()[0].get()),
+ argument_names));
+ for (int i = 1; i < expr->children().size(); ++i) {
+
RETURN_IF_ERROR(_set_comparator_argument_gap(expr->children()[i],
argument_names));
+ }
+ return Status::OK();
+ }
+
+ for (const auto& child : expr->children()) {
+ RETURN_IF_ERROR(_set_comparator_argument_gap(child,
argument_names));
+ }
+ return Status::OK();
+ }
+
+ Status _reject_nested_lambda_capture_of_comparator_argument(
+ const VLambdaFunctionExpr* lambda_expr,
+ const std::vector<std::string>* comparator_argument_names) const {
+ if (!lambda_expr->has_argument_names()) {
Review Comment:
This has the same mixed-version compatibility problem for `array_sort`
comparator bodies. The new thrift field is optional, but this branch makes it
mandatory for every nested lambda under the comparator: an old FE sends no
`lambda_argument_names`, and the new BE returns `InternalError` before checking
whether the nested body actually captures `x`/`y`. That can reject local-only
nested expressions, for example a comparator that uses `array_map(z -> z, ...)`
only to compute its scalar comparison value. Please keep a legacy validation
path for no-metadata nested lambdas that do not capture comparator arguments,
or gate this metadata-dependent validation on a compatible FE/BE
protocol/version.
--
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]
