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

NOT operator should not return internal error when args are not boolean value #8965

Closed
haohuaijin opened this issue Jan 23, 2024 · 4 comments · Fixed by #8982
Closed

NOT operator should not return internal error when args are not boolean value #8965

haohuaijin opened this issue Jan 23, 2024 · 4 comments · Fixed by #8982
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@haohuaijin
Copy link
Contributor

Describe the bug

❯ select not('hi');
Internal error: NOT 'Literal { value: Utf8("hi") }' can't be evaluated because the expression's type is Utf8, not boolean or NULL.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker
❯ select 1 and 2;
Error during planning: Cannot infer common argument type for logical boolean operation Int64 AND Int64

I think the return error message of NOT should like And operator, they both only work for boolean value.

Originally posted by @haohuaijin in #8957 (comment)

To Reproduce

No response

Expected behavior

No response

Additional context

No response

@haohuaijin haohuaijin added the bug Something isn't working label Jan 23, 2024
@alamb alamb added good first issue Good for newcomers and removed good first issue Good for newcomers labels Jan 23, 2024
@alamb
Copy link
Contributor

alamb commented Jan 23, 2024

Ideally, something early in the code (perhaps the type coercion logic) should return an error if the argument to not can't be coerced to Boolean

It looks like perhaps we need to add a clause to
https://github.com/apache/arrow-datafusion/blob/af3d19038ef40e45c70c6e9a28798ee53795dfe9/datafusion/optimizer/src/analyzer/type_coercion.rs#L179-L182

Perhaps following the model of Expr::IsNotNull

As @haohuaijin notes we may want to update https://github.com/apache/arrow-datafusion/blob/e642cc2a94f38518d765d25c8113523aedc29198/datafusion/physical-expr/src/expressions/not.rs#L83-L92 to not check for is_null

@alamb alamb added the good first issue Good for newcomers label Jan 23, 2024
@alamb
Copy link
Contributor

alamb commented Jan 23, 2024

I think this is a good first issue -- the code should be a matter of following existing patterns and ensuring there is some tests for this

@guojidan
Copy link
Contributor

I want try implement this 😄

@guojidan
Copy link
Contributor

As @haohuaijin notes we may want to update

https://github.com/apache/arrow-datafusion/blob/e642cc2a94f38518d765d25c8113523aedc29198/datafusion/physical-expr/src/expressions/not.rs#L83-L92

to not check for is_null

I think we still need check is_null, because get_casted_expr_for_bool_op will convert null to ScalarValue::Boolean(NULL), but ScalarValue::Boolean(NULL) can not try_into() to bool type, so it will throw an error: Internal error: Cannot convert Boolean(NULL) to bool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants