Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Expression ExecuteScalarExpression execute empty args function with a wrong result #39860

Closed
ZhangHuiGui opened this issue Jan 31, 2024 · 0 comments · Fixed by #39908
Closed
Assignees
Milestone

Comments

@ZhangHuiGui
Copy link
Collaborator

ZhangHuiGui commented Jan 31, 2024

Describe the bug, including details regarding any error messages, version, and platform.

Problem

We have some arrow-builtin functions with no arguments support, like random/count_all/hash_count_all.

When we use call("random", {}, opts) and execute the expression with ExecuteScalarExpression(random_expr, cp::ExecBatch({}, 10)), the result only output one row. What we expected is 10 rows array.

 [
  0.5353881964857193
]

Reproduce

TEST(Expression, ExprWithNoArguments) {
   auto random_options = RandomOptions::FromSeed(/*seed=*/0);
   auto expr = call("random", {}, random_opts);
   ASSERT_OK_AND_ASSIGN(expr, expr.Bind(arrow::float64()));
  ASSERT_OK_AND_ASSIGN(auto out, ExecuteScalarExpression(
                                        expr, ExecBatch({}, 10)));
   std::cout << out.make_array()->ToString() << std::endl; // expect 10 rows but only get 1 row
}

Reason

The ExecuteScalarExpression internal code do some checks like below:

  bool all_scalar = true;
  for (size_t i = 0; i < arguments.size(); ++i) {
    ARROW_ASSIGN_OR_RAISE(
        arguments[i], ExecuteScalarExpression(call->arguments[i], input, exec_context));
    if (arguments[i].is_array()) {
      all_scalar = false;
    }
  }

The all_scalar will decide the output rows, if all_scalar is true, only output 1 row.
So we should consider the empty function arguments here? Or i missed some background here?

Component(s)

C++

ZhangHuiGui pushed a commit to ZhangHuiGui/arrow that referenced this issue Feb 2, 2024
bkietz pushed a commit that referenced this issue Feb 5, 2024
… function with a wrong result (#39908)

### Rationale for this change

Try to fix #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: #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>
@bkietz bkietz added this to the 16.0.0 milestone Feb 5, 2024
@pitrou pitrou modified the milestones: 16.0.0, 15.0.1 Feb 14, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…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>
raulcd pushed a commit that referenced this issue Feb 20, 2024
… function with a wrong result (#39908)

### Rationale for this change

Try to fix #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: #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>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
…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>
mdianjun pushed a commit to marsno1/arrow that referenced this issue Mar 1, 2024
…y args function with a wrong result (apache#39908)

Try to fix apache#39860.

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

Yes

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>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants