Skip to content

Conversation

@vibhatha
Copy link
Contributor

@vibhatha vibhatha commented Jul 7, 2023

Rationale for this change

The change is bases on a user filed issue. This would provide a readable error message.

What changes are included in this PR?

Did modification to hash_join_node.cc to include field name to the already provided error message.
Added python tests to validate the response.

Are these changes tested?

A test case has been added to Python under test_table.py

Are there any user-facing changes?

No, just a clear error message is provided.

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

This looks good to me modulo a few very minor nits.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 7, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 12, 2023
@vibhatha vibhatha marked this pull request as ready for review July 12, 2023 04:12
@westonpace westonpace merged commit 95e3e77 into apache:main Jul 12, 2023
@westonpace westonpace removed the awaiting change review Awaiting change review label Jul 12, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Jul 12, 2023
np.testing.assert_allclose(result, expected)
assert result.dtype == "int32"


Copy link
Member

Choose a reason for hiding this comment

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

We need to add @pytest.mark.acero because this depends on Acero.

I'm adding it in #36681 but we may want to do it in a separated PR only for it...

Copy link
Member

Choose a reason for hiding this comment

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

I've split the fix to #36683.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jul 14, 2023
@conbench-apache-arrow
Copy link

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

There were 3 benchmark results indicating a performance regression:

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Display the name of the problematic field when returning status "Data type ... is not supported in join non-key field" for HashJoin

3 participants