Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/dialect/duckdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

use crate::dialect::Dialect;
use crate::dialect::{Dialect, IsNotNullAlias};

/// A [`Dialect`] for [DuckDB](https://duckdb.org/)
#[derive(Debug, Default)]
Expand Down Expand Up @@ -94,4 +94,13 @@ impl Dialect for DuckDbDialect {
fn supports_order_by_all(&self) -> bool {
true
}

/// DuckDB supports `NOT NULL` and `NOTNULL` as aliases
/// for `IS NOT NULL`, see DuckDB Comparisons <https://duckdb.org/docs/stable/sql/expressions/comparison_operators#between-and-is-not-null>
fn supports_is_not_null_alias(&self, alias: IsNotNullAlias) -> bool {
match alias {
IsNotNullAlias::NotNull => true,
IsNotNullAlias::NotSpaceNull => true,
}
}
}
28 changes: 28 additions & 0 deletions src/dialect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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))
}
Comment on lines +654 to +666
Copy link
Contributor

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 lacking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 637fb69

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)),
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

match alias {
NotNull => false,
NotSpaceNull => false,
}
}
}

/// This represents the operators for which precedence must be defined
Expand All @@ -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 {
Expand Down
11 changes: 10 additions & 1 deletion src/dialect/postgresql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
// limitations under the License.
use log::debug;

use crate::dialect::{Dialect, Precedence};
use crate::dialect::{Dialect, IsNotNullAlias, Precedence};
use crate::keywords::Keyword;
use crate::parser::{Parser, ParserError};
use crate::tokenizer::Token;
Expand Down Expand Up @@ -262,4 +262,13 @@ impl Dialect for PostgreSqlDialect {
fn supports_alter_column_type_using(&self) -> bool {
true
}

/// Postgres supports `NOTNULL` as an alias for `IS NOT NULL`
/// but does not support `NOT NULL`. See: <https://www.postgresql.org/docs/17/functions-comparison.html>
fn supports_is_not_null_alias(&self, alias: IsNotNullAlias) -> bool {
match alias {
IsNotNullAlias::NotNull => true,
IsNotNullAlias::NotSpaceNull => false,
}
}
}
11 changes: 10 additions & 1 deletion src/dialect/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use alloc::boxed::Box;

use crate::ast::BinaryOperator;
use crate::ast::{Expr, Statement};
use crate::dialect::Dialect;
use crate::dialect::{Dialect, IsNotNullAlias};
use crate::keywords::Keyword;
use crate::parser::{Parser, ParserError};

Expand Down Expand Up @@ -110,4 +110,13 @@ impl Dialect for SQLiteDialect {
fn supports_dollar_placeholder(&self) -> bool {
true
}

/// SQLite supports ``NOT NULL` and `NOTNULL` as
/// aliases for `IS NOT NULL`, see: <https://sqlite.org/syntax/expr.html>
fn supports_is_not_null_alias(&self, alias: IsNotNullAlias) -> bool {
match alias {
IsNotNullAlias::NotNull => true,
IsNotNullAlias::NotSpaceNull => true,
}
}
}
1 change: 1 addition & 0 deletions src/keywords.rs
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,7 @@ define_keywords!(
NOT,
NOTHING,
NOTIFY,
NOTNULL,
NOWAIT,
NO_WRITE_TO_BINLOG,
NTH_VALUE,
Expand Down
7 changes: 7 additions & 0 deletions src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use IsOptional::*;
use crate::ast::helpers::stmt_create_table::{CreateTableBuilder, CreateTableConfiguration};
use crate::ast::Statement::CreatePolicy;
use crate::ast::*;
use crate::dialect::IsNotNullAlias::{NotNull, NotSpaceNull};
use crate::dialect::*;
use crate::keywords::{Keyword, ALL_KEYWORDS};
use crate::tokenizer::*;
Expand Down Expand Up @@ -3562,6 +3563,7 @@ impl<'a> Parser<'a> {
let negated = self.parse_keyword(Keyword::NOT);
let regexp = self.parse_keyword(Keyword::REGEXP);
let rlike = self.parse_keyword(Keyword::RLIKE);
let null = self.parse_keyword(Keyword::NULL);
if regexp || rlike {
Ok(Expr::RLike {
negated,
Expand All @@ -3571,6 +3573,8 @@ impl<'a> Parser<'a> {
),
regexp,
})
} else if dialect.supports_is_not_null_alias(NotSpaceNull) && negated && null {
Ok(Expr::IsNotNull(Box::new(expr)))
} else if self.parse_keyword(Keyword::IN) {
self.parse_in(expr, negated)
} else if self.parse_keyword(Keyword::BETWEEN) {
Expand Down Expand Up @@ -3608,6 +3612,9 @@ impl<'a> Parser<'a> {
self.expected("IN or BETWEEN after NOT", self.peek_token())
}
}
Keyword::NOTNULL if dialect.supports_is_not_null_alias(NotNull) => {
Ok(Expr::IsNotNull(Box::new(expr)))
}
Keyword::MEMBER => {
if self.parse_keyword(Keyword::OF) {
self.expect_token(&Token::LParen)?;
Expand Down
134 changes: 132 additions & 2 deletions tests/sqlparser_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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"#;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 verified_expr() helper functions to make expression testing easier)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!(),
}
}