DuckDB, Postgres, SQLite: NOT NULL and NOTNULL expressions #1927
Conversation
|
|
||
| /// Returns true if the dialect supports the passed in alias. | ||
| /// See [IsNotNullAlias]. | ||
| fn supports_is_not_null_alias(&self, alias: IsNotNullAlias) -> bool { |
There was a problem hiding this comment.
would it make sense to have this behavior dialect agnostic and let the parser always accept any variant that shows up? it would let us skip this dialect method as a result
There was a problem hiding this comment.
Hmm, the main issue I see w/ that approach is that technically x NOTNULL is shorthand for x AS NOTNULL for dialects that don't support NOTNULL but do support aliases w/o AS, so this could be considered a breaking change if a query happened to by using NOTNULL as a column alias.
| Token::Word(w) | ||
| if w.keyword == Keyword::NULL | ||
| && self.supports_is_not_null_alias(NotSpaceNull) => | ||
| { | ||
| Ok(p!(Is)) | ||
| } | ||
| _ => Ok(self.prec_unknown()), | ||
| }, | ||
| Token::Word(w) | ||
| if w.keyword == Keyword::NOTNULL && self.supports_is_not_null_alias(NotNull) => | ||
| { | ||
| Ok(p!(Is)) | ||
| } |
There was a problem hiding this comment.
for the new operators can we add a test case that cover their precedence behavior? see here for example. It would be good if we don't already have coverage to also ensure that we aren't also inadvertently changing the precedence of the IS NOT NULL operator as well, so we could also include a test case demonstrating that if lacking
There was a problem hiding this comment.
Added in 637fb69
tests/sqlparser_common.rs
Outdated
| // All other dialects consider `x NOTNULL` like `x AS NOTNULL` and thus | ||
| // consider `NOTNULL` an alias for x. | ||
| let sql = r#"WITH t AS (SELECT NULL AS x) SELECT x NOTNULL FROM t"#; | ||
| let canonical = r#"WITH t AS (SELECT NULL AS x) SELECT x AS NOTNULL FROM t"#; |
There was a problem hiding this comment.
I think we can simplify this test, since we're only interested in testing the expression, doing so via a CTE makes the test verbose and harder to maintain going forward without providing additional coverage (note we also have verified_expr() helper functions to make expression testing easier)
There was a problem hiding this comment.
Thanks! I simplified the tests in 431fca3, thanks for the pointers they are much more readable now!
Fixes #1920 by adding
NOT NULLexpression support to DuckDB and SQLite, andNOTNULLsupport to DuckDB, SQLite, and Postgres.Since this is my first non-trivial PR please provide any feedback! I confirmed all tests pass w/
cargo testandcargo test --all-featuresbut if there's anything else I need to do please let me know, thanks!