Skip to content

Commit

Permalink
apacheGH-39860: [C++] Expression ExecuteScalarExpression execute empt…
Browse files Browse the repository at this point in the history
…y args function with a wrong result (apache#39908)

### Rationale for this change

Try to fix apache#39860.

### What changes are included in this PR?

Deal with the call->arguments.size() == 0's condition in ExecuteScalarExpression when we call some functions
has no arguments, like (random, hash_count ...).

### Are these changes tested?

Yes

### Are there any user-facing changes?

No.
* Closes: apache#39860

Lead-authored-by: hugo.zhang <hugo.zhang@openpie.com>
Co-authored-by: 张回归 <zhanghuigui@zhanghuiguideMacBook-Pro-1681.local>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
  • Loading branch information
2 people authored and zanmato1984 committed Feb 28, 2024
1 parent fb0d129 commit 869fa1e
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 2 deletions.
13 changes: 11 additions & 2 deletions cpp/src/arrow/compute/expression.cc
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,15 @@ Result<Datum> ExecuteScalarExpression(const Expression& expr, const ExecBatch& i
}
}

int64_t input_length;
if (!arguments.empty() && all_scalar) {
// all inputs are scalar, so use a 1-long batch to avoid
// computing input.length equivalent outputs
input_length = 1;
} else {
input_length = input.length;
}

auto executor = compute::detail::KernelExecutor::MakeScalar();

compute::KernelContext kernel_context(exec_context, call->kernel);
Expand All @@ -772,8 +781,8 @@ Result<Datum> ExecuteScalarExpression(const Expression& expr, const ExecBatch& i
RETURN_NOT_OK(executor->Init(&kernel_context, {kernel, types, options}));

compute::detail::DatumAccumulator listener;
RETURN_NOT_OK(executor->Execute(
ExecBatch(std::move(arguments), all_scalar ? 1 : input.length), &listener));
RETURN_NOT_OK(
executor->Execute(ExecBatch(std::move(arguments), input_length), &listener));
const auto out = executor->WrapResults(arguments, listener.values());
#ifndef NDEBUG
DCHECK_OK(executor->CheckResultType(out, call->function_name.c_str()));
Expand Down
19 changes: 19 additions & 0 deletions cpp/src/arrow/compute/expression_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,25 @@ TEST(Expression, ExecuteCall) {
])"));
}

TEST(Expression, ExecuteCallWithNoArguments) {
const int kCount = 10;
auto random_options = RandomOptions::FromSeed(/*seed=*/0);
ExecBatch input({}, kCount);

Expression random_expr = call("random", {}, random_options);
ASSERT_OK_AND_ASSIGN(random_expr, random_expr.Bind(float64()));

ASSERT_OK_AND_ASSIGN(Datum actual, ExecuteScalarExpression(random_expr, input));
compute::ExecContext* exec_context = default_exec_context();
ASSERT_OK_AND_ASSIGN(auto function,
exec_context->func_registry()->GetFunction("random"));
ASSERT_OK_AND_ASSIGN(Datum expected,
function->Execute(input, &random_options, exec_context));
AssertDatumsEqual(actual, expected, /*verbose=*/true);

EXPECT_EQ(actual.length(), kCount);
}

TEST(Expression, ExecuteDictionaryTransparent) {
ExpectExecute(
equal(field_ref("a"), field_ref("b")),
Expand Down

0 comments on commit 869fa1e

Please sign in to comment.