-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support NULL
literals in where clause
#11266
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
Conversation
}, | ||
Err(_) => { | ||
let Ok(null_array) = as_null_array(&array) else { | ||
return internal_err!("Cannot create filter_array from non-boolean predicates, unable to continute"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous it will throw a downcast_error, which mess up the debugging logs.
@@ -24,3 +24,17 @@ query TT? | |||
select 'foo', '', NULL | |||
---- | |||
foo (empty) NULL | |||
|
|||
# Where clause accept NULL literal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From postgres,
postgres=# select 1 where null and 1=1;
?column?
----------
(0 rows)
postgres=# select 1 where null and 1=1;
?column?
----------
(0 rows)
postgres=# select 1 where null or 1=1;
?column?
----------
1
(1 row)
NULL
literals in where clause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xinlifoobar -- this makes sense to me
Another potential way to fix this issue would be to cast / coerce NULL literals in the WHERE clause to boolean
Basically we could add the equivalent of NULL::boolean
For example, given the input
SELECT ... FROM foo WHERE NULL and x = 5;
We could transform it to
SELECT ... FROM foo WHERE NULL::boolean and x = 5;
That might result in simpler code as FilterExec would not have to be aware of DataType::null
Perhaps it would fit in https://github.com/apache/datafusion/blob/main/datafusion/optimizer/src/analyzer/type_coercion.rs
However, I think this PR is better than main and we could try to see if the alternate approach loked better as a future work (or never)
Thanks again @xinlifoobar |
This is a lot... Let me looking into this. |
* Try fix where clause incorrectly reject NULL literal * check null in filter
This reverts commit fa01917.
* Try fix where clause incorrectly reject NULL literal * check null in filter
…pache#11491) * Revert "Support `NULL` literals in where clause (apache#11266)" This reverts commit fa01917. * Followup Support NULL literals in where clause * misc err change * adopt comparison_coercion * Fix comments * Fix comments
…pache#11491) * Revert "Support `NULL` literals in where clause (apache#11266)" This reverts commit fa01917. * Followup Support NULL literals in where clause * misc err change * adopt comparison_coercion * Fix comments * Fix comments
Which issue does this PR close?
Closes #11248
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?