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

[Minor] Improve error message when bitwise_* operator takes wrong unsupported type #12646

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

dharanad
Copy link
Contributor

Which issue does this PR close?

Closes #11249

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Sep 27, 2024
@dharanad dharanad marked this pull request as ready for review September 27, 2024 10:44
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @dharanad -- this looks much better than what is currently done. It might be better to use not_impl_err but we could always do that as a follow on PR or never too

Thanks again!

@@ -24,7 +24,7 @@ use arrow::compute::kernels::bitwise::{
bitwise_xor, bitwise_xor_scalar,
};
use arrow::datatypes::DataType;
use datafusion_common::internal_err;
use datafusion_common::plan_err;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps not_impl_err would be more appropriate?

https://docs.rs/datafusion/latest/datafusion/common/macro.not_impl_err.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems unlikely that we would support such an operation so plan_err should be good right ? However, I may be overlooking some factors. Could you please clarify if there's a specific rationale behind this suggestion that I might have missed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that most plan_err s are issues with a user's query (some bug they should fix) where as not_impl signals that there isn't some bug with the query

This is kind of documented here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats clear. Thank You

@alamb alamb merged commit f87db21 into apache:main Sep 28, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 28, 2024

Plan error is certainly better than internal error. We can consider switching to not implemented error as a follow on PR

Thanks @dharanad

@dharanad dharanad deleted the fix/11249 branch September 29, 2024 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make error message better when bitwise_* operator takes wrong argument type
2 participants