-
Notifications
You must be signed in to change notification settings - Fork 621
DuckDB, Postgres, SQLite: NOT NULL and NOTNULL expressions #1927
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
d6f51a1
f022dfa
2bf08db
431fca3
637fb69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ use crate::keywords::Keyword; | |
use crate::parser::{Parser, ParserError}; | ||
use crate::tokenizer::Token; | ||
|
||
use crate::dialect::IsNotNullAlias::{NotNull, NotSpaceNull}; | ||
#[cfg(not(feature = "std"))] | ||
use alloc::boxed::Box; | ||
|
||
|
@@ -650,8 +651,19 @@ pub trait Dialect: Debug + Any { | |
Token::Word(w) if w.keyword == Keyword::MATCH => Ok(p!(Like)), | ||
Token::Word(w) if w.keyword == Keyword::SIMILAR => Ok(p!(Like)), | ||
Token::Word(w) if w.keyword == Keyword::MEMBER => Ok(p!(Like)), | ||
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)) | ||
} | ||
Token::Word(w) if w.keyword == Keyword::IS => Ok(p!(Is)), | ||
Token::Word(w) if w.keyword == Keyword::IN => Ok(p!(Between)), | ||
Token::Word(w) if w.keyword == Keyword::BETWEEN => Ok(p!(Between)), | ||
|
@@ -1076,6 +1088,15 @@ pub trait Dialect: Debug + Any { | |
fn supports_comma_separated_drop_column_list(&self) -> bool { | ||
false | ||
} | ||
|
||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the main issue I see w/ that approach is that technically |
||
match alias { | ||
NotNull => false, | ||
NotSpaceNull => false, | ||
} | ||
} | ||
} | ||
|
||
/// This represents the operators for which precedence must be defined | ||
|
@@ -1102,6 +1123,13 @@ pub enum Precedence { | |
Or, | ||
} | ||
|
||
/// Possible aliases for `IS NOT NULL` supported | ||
/// by some non-standard dialects. | ||
pub enum IsNotNullAlias { | ||
NotNull, | ||
NotSpaceNull, | ||
} | ||
|
||
impl dyn Dialect { | ||
#[inline] | ||
pub fn is<T: Dialect>(&self) -> bool { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -608,6 +608,7 @@ define_keywords!( | |
NOT, | ||
NOTHING, | ||
NOTIFY, | ||
NOTNULL, | ||
NOWAIT, | ||
NO_WRITE_TO_BINLOG, | ||
NTH_VALUE, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,8 +32,8 @@ use sqlparser::ast::TableFactor::{Pivot, Unpivot}; | |
use sqlparser::ast::*; | ||
use sqlparser::dialect::{ | ||
AnsiDialect, BigQueryDialect, ClickHouseDialect, DatabricksDialect, Dialect, DuckDbDialect, | ||
GenericDialect, HiveDialect, MsSqlDialect, MySqlDialect, PostgreSqlDialect, RedshiftSqlDialect, | ||
SQLiteDialect, SnowflakeDialect, | ||
GenericDialect, HiveDialect, IsNotNullAlias, MsSqlDialect, MySqlDialect, PostgreSqlDialect, | ||
RedshiftSqlDialect, SQLiteDialect, SnowflakeDialect, | ||
}; | ||
use sqlparser::keywords::{Keyword, ALL_KEYWORDS}; | ||
use sqlparser::parser::{Parser, ParserError, ParserOptions}; | ||
|
@@ -15974,3 +15974,133 @@ fn parse_create_procedure_with_parameter_modes() { | |
_ => unreachable!(), | ||
} | ||
} | ||
|
||
#[test] | ||
fn parse_not_null_unsupported() { | ||
// Only DuckDB and SQLite support `x NOT NULL` as an expression | ||
// All other dialects fail to parse. | ||
let sql = r#"WITH t AS (SELECT NULL AS x) SELECT x NOT NULL FROM t"#; | ||
let dialects = | ||
all_dialects_except(|d| d.supports_is_not_null_alias(IsNotNullAlias::NotSpaceNull)); | ||
let res = dialects.parse_sql_statements(sql); | ||
assert_eq!( | ||
ParserError::ParserError("Expected: end of statement, found: NULL".to_string()), | ||
res.unwrap_err() | ||
); | ||
} | ||
|
||
#[test] | ||
fn parse_not_null_supported() { | ||
// DuckDB and SQLite support `x NOT NULL` as an alias for `x IS NOT NULL` | ||
let sql = r#"WITH t AS (SELECT NULL AS x) SELECT x NOT NULL FROM t"#; | ||
let canonical = r#"WITH t AS (SELECT NULL AS x) SELECT x IS NOT NULL FROM t"#; | ||
let dialects = | ||
all_dialects_where(|d| d.supports_is_not_null_alias(IsNotNullAlias::NotSpaceNull)); | ||
let stmt = dialects.one_statement_parses_to(sql, canonical); | ||
match stmt { | ||
Statement::Query(qry) => match *qry.body { | ||
SetExpr::Select(select) => { | ||
assert_eq!(select.projection.len(), 1); | ||
match select.projection.first().unwrap() { | ||
UnnamedExpr(expr) => { | ||
let fake_span = Span { | ||
start: Location { line: 0, column: 0 }, | ||
end: Location { line: 0, column: 0 }, | ||
}; | ||
assert_eq!( | ||
*expr, | ||
Expr::IsNotNull(Box::new(Identifier(Ident { | ||
value: "x".to_string(), | ||
quote_style: None, | ||
span: fake_span, | ||
})),), | ||
); | ||
} | ||
_ => unreachable!(), | ||
} | ||
} | ||
_ => unreachable!(), | ||
}, | ||
_ => unreachable!(), | ||
} | ||
} | ||
|
||
#[test] | ||
fn parse_notnull_unsupported() { | ||
// Only Postgres, DuckDB, and SQLite support `x NOTNULL` as an expression | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I simplified the tests in 431fca3, thanks for the pointers they are much more readable now! |
||
let dialects = all_dialects_except(|d| d.supports_is_not_null_alias(IsNotNullAlias::NotNull)); | ||
let stmt = dialects.one_statement_parses_to(sql, canonical); | ||
match stmt { | ||
Statement::Query(qry) => match *qry.body { | ||
SetExpr::Select(select) => { | ||
assert_eq!(select.projection.len(), 1); | ||
match select.projection.first().unwrap() { | ||
SelectItem::ExprWithAlias { expr, alias } => { | ||
let fake_span = Span { | ||
start: Location { line: 0, column: 0 }, | ||
end: Location { line: 0, column: 0 }, | ||
}; | ||
assert_eq!( | ||
*expr, | ||
Identifier(Ident { | ||
value: "x".to_string(), | ||
quote_style: None, | ||
span: fake_span, | ||
}) | ||
); | ||
assert_eq!( | ||
*alias, | ||
Ident { | ||
value: "NOTNULL".to_string(), | ||
quote_style: None, | ||
span: fake_span, | ||
} | ||
); | ||
} | ||
_ => unreachable!(), | ||
} | ||
} | ||
_ => unreachable!(), | ||
}, | ||
_ => unreachable!(), | ||
} | ||
} | ||
|
||
#[test] | ||
fn parse_notnull_supported() { | ||
// Postgres, DuckDB and SQLite support `x NOTNULL` as an alias for `x IS NOT NULL` | ||
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 IS NOT NULL FROM t"#; | ||
let dialects = all_dialects_where(|d| d.supports_is_not_null_alias(IsNotNullAlias::NotNull)); | ||
let stmt = dialects.one_statement_parses_to(sql, canonical); | ||
match stmt { | ||
Statement::Query(qry) => match *qry.body { | ||
SetExpr::Select(select) => { | ||
assert_eq!(select.projection.len(), 1); | ||
match select.projection.first().unwrap() { | ||
UnnamedExpr(expr) => { | ||
let fake_span = Span { | ||
start: Location { line: 0, column: 0 }, | ||
end: Location { line: 0, column: 0 }, | ||
}; | ||
assert_eq!( | ||
*expr, | ||
Expr::IsNotNull(Box::new(Identifier(Ident { | ||
value: "x".to_string(), | ||
quote_style: None, | ||
span: fake_span, | ||
})),), | ||
); | ||
} | ||
_ => unreachable!(), | ||
} | ||
} | ||
_ => unreachable!(), | ||
}, | ||
_ => unreachable!(), | ||
} | ||
} |
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.
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 lackingThere 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.
Added in 637fb69