iffyio commented on code in PR #1927:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1927#discussion_r2192730960
##########
src/dialect/mod.rs:
##########
@@ -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 {
Review 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
##########
src/dialect/mod.rs:
##########
@@ -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))
+ }
Review Comment:
for the new operators can we add a test case that cover their precedence
behavior? see
[here](https://github.com/apache/datafusion-sqlparser-rs/blob/2bf08db93d994b056820f747f3a6335e3046738a/tests/sqlparser_common.rs#L1893)
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
##########
tests/sqlparser_common.rs:
##########
@@ -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"#;
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
