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++] Cast function bind failed after add a alias name through AddAlias #40183

Closed
ZhangHuiGui opened this issue Feb 21, 2024 · 1 comment
Closed
Assignees
Milestone

Comments

@ZhangHuiGui
Copy link
Collaborator

ZhangHuiGui commented Feb 21, 2024

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

How to reproduce

auto fm = arrow::compute::GetFunctionRegistry();
ASSERT_OK(fm->AddAlias("alias_cast", "cast"));
auto s1 = arrow::schema({arrow::field("f1", arrow::decimal128(30, 3))});
auto b = arrow::RecordBatchFromJSON(s1, R"([
    ["1.000"],
    ["-4567.890"]
])");
auto expr = arrow::compute::call("alias_cast", {arrow::compute::field_ref("f1")}, 
        arrow::compute::CastOptions::Unsafe(arrow::int32()));
ASSERT_OK_AND_ASSIGN(expr, expr.Bind(*s1)); // Bind failed

ASSERT_OK_AND_ASSIGN(auto input, arrow::compute::MakeExecBatch(*s1, b));
ASSERT_OK_AND_ASSIGN(auto res_dat,
                     arrow::compute::ExecuteScalarExpression(expr, input));

Bind failed with NotImplemented: Dispatch for a MetaFunction's Kernels.

Reason

BindNonRecursive will call arrow::compute::GetFunction, the function handle the cast with specific name and
return an original function object rather than CastFunction which override the DispatchExact.

inline Result<std::shared_ptr<compute::Function>> GetFunction(
    const Expression::Call& call, compute::ExecContext* exec_context) {
  if (call.function_name != "cast") {
    return exec_context->func_registry()->GetFunction(call.function_name);
  }
  // XXX this special case is strange; why not make "cast" a ScalarFunction?
  const TypeHolder& to_type =
      ::arrow::internal::checked_cast<const compute::CastOptions&>(*call.options).to_type;
  return GetCastFunction(*to_type);
}

It's a huge project for us to unify the CastFunction schedule path to standard ScalarFunction, so i think maybe we should ban AddAlias for cast?

Component(s)

C++

bkietz pushed a commit that referenced this issue Mar 4, 2024
… through AddAlias (#40200)

### Rationale for this change

Cast function bind failed after add a alias name through AddAlias.

### What changes are included in this PR?

Add a const `cast_function` which registered in `AddFunction` for check cast alias in `arrow::compute::GetFunction`.

### Are these changes tested?
Yes

### Are there any user-facing changes?
Yes, cast's alias name can also execute with expression system.

* GitHub Issue: #40183

Authored-by: hugo.zhang <hugo.zhang@openpie.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
@bkietz bkietz added this to the 16.0.0 milestone Mar 4, 2024
@bkietz
Copy link
Member

bkietz commented Mar 4, 2024

Issue resolved by pull request 40200
#40200

@bkietz bkietz closed this as completed Mar 4, 2024
mapleFU pushed a commit to mapleFU/arrow that referenced this issue Mar 7, 2024
…s name through AddAlias (apache#40200)

### Rationale for this change

Cast function bind failed after add a alias name through AddAlias.

### What changes are included in this PR?

Add a const `cast_function` which registered in `AddFunction` for check cast alias in `arrow::compute::GetFunction`.

### Are these changes tested?
Yes

### Are there any user-facing changes?
Yes, cast's alias name can also execute with expression system.

* GitHub Issue: apache#40183

Authored-by: hugo.zhang <hugo.zhang@openpie.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…s name through AddAlias (apache#40200)

### Rationale for this change

Cast function bind failed after add a alias name through AddAlias.

### What changes are included in this PR?

Add a const `cast_function` which registered in `AddFunction` for check cast alias in `arrow::compute::GetFunction`.

### Are these changes tested?
Yes

### Are there any user-facing changes?
Yes, cast's alias name can also execute with expression system.

* GitHub Issue: apache#40183

Authored-by: hugo.zhang <hugo.zhang@openpie.com>
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
Development

No branches or pull requests

2 participants