Skip to content

GH-47268: [C++][Compute] Fix discarded bad status for call binding - #47284

Merged
zanmato1984 merged 2 commits into
apache:mainfrom
zanmato1984:fix/gh-47268
Aug 28, 2025
Merged

GH-47268: [C++][Compute] Fix discarded bad status for call binding#47284
zanmato1984 merged 2 commits into
apache:mainfrom
zanmato1984:fix/gh-47268

Conversation

@zanmato1984

@zanmato1984zanmato1984 commented Aug 8, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

Faithfully propagate the bad status thrown in call expression binding.

What changes are included in this PR?

Early return when status is bad.

Are these changes tested?

UT included.

Are there any user-facing changes?

None.

@github-actions

github-actionsBot commented Aug 8, 2025

Copy link
Copy Markdown

⚠️ GitHub issue #47268has been automatically assigned in GitHub to PR creator.

@zanmato1984

zanmato1984 commented Aug 8, 2025

Copy link
Copy Markdown
ContributorAuthor

Hmm, the failed test is related. However I don't think this is a regression. Instead, I would say this reveals a fix (#40223) that should be done in a better way. I'll open a separate issue for this and keep this PR until then.

@zanmato1984zanmato1984 mentioned this pull request Aug 8, 2025
Closed
@zanmato1984

zanmato1984 commented Aug 8, 2025

Copy link
Copy Markdown
ContributorAuthor

#47287 filed to rework the fix #40223 and address the regressed test. Then this fix itself should pass CI.

zanmato1984
zanmato1984 commented Aug 20, 2025
View reviewed changes
Comment threadcpp/src/arrow/compute/expression.cc
std::vector<TypeHolder> types = GetTypes(call.arguments);
ARROW_ASSIGN_OR_RAISE(call.function, GetFunction(call, exec_context));

auto FinishBind = [&] {

@zanmato1984zanmato1984Aug 20, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically reverting the change of #40223. Because we've reworked the fix in #47297. Besides, the original control flow (prior to #40223) is much more straightforward.

@bkietzbkietzAug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for working on this!

@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 20, 2025
@zanmato1984

zanmato1984 commented Aug 20, 2025

Copy link
Copy Markdown
ContributorAuthor

Hi @pitrou, me again. Would you like to take a look? The change is basically reverting #40223 with a dedicated test. Thanks.

@zanmato1984

zanmato1984 commented Aug 20, 2025

Copy link
Copy Markdown
ContributorAuthor

@github-actions crossbow submit -g cpp

@github-actions

github-actionsBot commented Aug 20, 2025

Copy link
Copy Markdown

Revision: 2e340ab

Submitted crossbow builds: ursacomputing/crossbow @ actions-ec4d388956

TaskStatus
example-cpp-minimal-build-staticGitHub Actions
example-cpp-minimal-build-static-system-dependencyGitHub Actions
example-cpp-tutorialGitHub Actions
test-build-cpp-fuzzGitHub Actions
test-conda-cppGitHub Actions
test-conda-cpp-valgrindGitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1GitHub Actions
test-debian-12-cpp-amd64GitHub Actions
test-debian-12-cpp-i386GitHub Actions
test-fedora-42-cppGitHub Actions
test-ubuntu-22.04-cppGitHub Actions
test-ubuntu-22.04-cpp-20GitHub Actions
test-ubuntu-22.04-cpp-bundledGitHub Actions
test-ubuntu-22.04-cpp-emscriptenGitHub Actions
test-ubuntu-22.04-cpp-no-threadingGitHub Actions
test-ubuntu-24.04-cppGitHub Actions
test-ubuntu-24.04-cpp-bundled-offlineGitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundledGitHub Actions
test-ubuntu-24.04-cpp-gcc-14GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formatsGitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizerGitHub Actions

@pitrou

pitrou commented Aug 20, 2025

Copy link
Copy Markdown
Member

@bkietz Are you available for a review here?

@zanmato1984

zanmato1984 commented Aug 28, 2025

Copy link
Copy Markdown
ContributorAuthor

Kindly ping @pitrou and @bkietz .

As explained in https://github.com/apache/arrow/pull/47284/files#r2288556722, this is a pure revert so can be easy to validate. Appreciate it!

bkietz
bkietz approved these changes Aug 28, 2025
View reviewed changes
Comment threadcpp/src/arrow/compute/expression.cc
std::vector<TypeHolder> types = GetTypes(call.arguments);
ARROW_ASSIGN_OR_RAISE(call.function, GetFunction(call, exec_context));

auto FinishBind = [&] {

@bkietzbkietzAug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for working on this!

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Aug 28, 2025
@zanmato1984
zanmato1984 merged commit 4d90dd7 into apache:mainAug 28, 2025
40 of 41 checks passed
@zanmato1984zanmato1984 removed the awaiting merge Awaiting merge label Aug 28, 2025
@zanmato1984zanmato1984 mentioned this pull request Aug 28, 2025
Closed
@conbench-apache-arrow

conbench-apache-arrowBot commented Aug 28, 2025

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4d90dd7.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
@zanmato1984@Mottl
apacheGH-47268: [C++][Compute] Fix discarded bad status for call bind…
7fddd2b
…ing (apache#47284)
### Rationale for this change
Faithfully propagate the bad status thrown in call expression binding.
### What changes are included in this PR?
Early return when status is bad.
### Are these changes tested?
UT included.
### Are there any user-facing changes?
None.
* GitHub Issue: apache#47268
Authored-by: Rossi Sun <zanmato1984@gmail.com>
Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietzbkietzbkietz approved these changes
@pitroupitrouAwaiting requested review from pitroupitrou is a code owner

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zanmato1984@pitrou@bkietz