From 1c2364142ce6d9ad574b018ec2e951f830707c42 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 09:03:29 +0100 Subject: [PATCH 1/9] WIP: parse true and false as identifiers in mssql fixes https://github.com/apache/datafusion-sqlparser-rs/issues/1508 --- src/dialect/mod.rs | 6 ++++++ src/dialect/mssql.rs | 5 +++++ src/parser/mod.rs | 14 +++++++++++--- tests/sqlparser_common.rs | 11 +++++++---- tests/sqlparser_mssql.rs | 12 ++++++++++++ 5 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/dialect/mod.rs b/src/dialect/mod.rs index 453fee3de..c4e19c44e 100644 --- a/src/dialect/mod.rs +++ b/src/dialect/mod.rs @@ -606,6 +606,12 @@ pub trait Dialect: Debug + Any { fn supports_top_before_distinct(&self) -> bool { false } + + /// Returns true if the dialect supports boolean literals (`true` and `false`). + /// For example, in MSSQL these are treated as identifiers rather than boolean literals. + fn supports_boolean_literals(&self) -> bool { + true + } } /// This represents the operators for which precedence must be defined diff --git a/src/dialect/mssql.rs b/src/dialect/mssql.rs index a5ee0bf75..8aab0bc8a 100644 --- a/src/dialect/mssql.rs +++ b/src/dialect/mssql.rs @@ -57,4 +57,9 @@ impl Dialect for MsSqlDialect { fn supports_try_convert(&self) -> bool { true } + + /// In MSSQL, there is no boolean type, and `true` and `false` are valid column names + fn supports_boolean_literals(&self) -> bool { + false + } } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 942ff19fd..790883437 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -1014,7 +1014,11 @@ impl<'a> Parser<'a> { let next_token = self.next_token(); let expr = match next_token.token { Token::Word(w) => match w.keyword { - Keyword::TRUE | Keyword::FALSE | Keyword::NULL => { + Keyword::TRUE | Keyword::FALSE if self.dialect.supports_boolean_literals() => { + self.prev_token(); + Ok(Expr::Value(self.parse_value()?)) + } + Keyword::NULL => { self.prev_token(); Ok(Expr::Value(self.parse_value()?)) } @@ -7571,8 +7575,12 @@ impl<'a> Parser<'a> { let location = next_token.location; match next_token.token { Token::Word(w) => match w.keyword { - Keyword::TRUE => Ok(Value::Boolean(true)), - Keyword::FALSE => Ok(Value::Boolean(false)), + Keyword::TRUE if self.dialect.supports_boolean_literals() => { + Ok(Value::Boolean(true)) + } + Keyword::FALSE if self.dialect.supports_boolean_literals() => { + Ok(Value::Boolean(false)) + } Keyword::NULL => Ok(Value::Null), Keyword::NoKeyword if w.quote_style.is_some() => match w.quote_style { Some('"') => Ok(Value::DoubleQuotedString(w.value)), diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index e37280636..c36ed09c2 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -7321,7 +7321,7 @@ fn lateral_derived() { let lateral_str = if lateral_in { "LATERAL " } else { "" }; let sql = format!( "SELECT * FROM customer LEFT JOIN {lateral_str}\ - (SELECT * FROM order WHERE order.customer = customer.id LIMIT 3) AS order ON true" + (SELECT * FROM orders WHERE orders.customer = customer.id LIMIT 3) AS orders ON 1" ); let select = verified_only_select(&sql); let from = only(select.from); @@ -7329,7 +7329,10 @@ fn lateral_derived() { let join = &from.joins[0]; assert_eq!( join.join_operator, - JoinOperator::LeftOuter(JoinConstraint::On(Expr::Value(Value::Boolean(true)))) + JoinOperator::LeftOuter(JoinConstraint::On(Expr::Value(Value::Number( + "1".to_string(), + false + )))) ); if let TableFactor::Derived { lateral, @@ -7338,10 +7341,10 @@ fn lateral_derived() { } = join.relation { assert_eq!(lateral_in, lateral); - assert_eq!(Ident::new("order"), alias.name); + assert_eq!(Ident::new("orders"), alias.name); assert_eq!( subquery.to_string(), - "SELECT * FROM order WHERE order.customer = customer.id LIMIT 3" + "SELECT * FROM orders WHERE orders.customer = customer.id LIMIT 3" ); } else { unreachable!() diff --git a/tests/sqlparser_mssql.rs b/tests/sqlparser_mssql.rs index c5f43b072..ad41b500c 100644 --- a/tests/sqlparser_mssql.rs +++ b/tests/sqlparser_mssql.rs @@ -1031,6 +1031,18 @@ fn parse_create_table_with_identity_column() { } } +#[test] +fn parse_true_false_as_identifiers() { + assert_eq!( + ms().verified_expr("true"), + Expr::Identifier(Ident::new("true")) + ); + assert_eq!( + ms().verified_expr("false"), + Expr::Identifier(Ident::new("false")) + ); +} + fn ms() -> TestedDialects { TestedDialects::new(vec![Box::new(MsSqlDialect {})]) } From c6a16a84d0588f372b0dcbda6d10527a3125893a Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 11:26:08 +0100 Subject: [PATCH 2/9] fix test_create_policy relying on all dialects having a "true" literal --- tests/sqlparser_common.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index c36ed09c2..766f473f6 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -11163,13 +11163,11 @@ fn parse_explain_with_option_list() { #[test] fn test_create_policy() { - let sql = concat!( - "CREATE POLICY my_policy ON my_table ", - "AS PERMISSIVE FOR SELECT ", - "TO my_role, CURRENT_USER ", - "USING (c0 = 1) ", - "WITH CHECK (true)" - ); + let sql: &str = "CREATE POLICY my_policy ON my_table \ + AS PERMISSIVE FOR SELECT \ + TO my_role, CURRENT_USER \ + USING (c0 = 1) \ + WITH CHECK (1 = 1)"; match all_dialects().verified_stmt(sql) { Statement::CreatePolicy { @@ -11197,7 +11195,14 @@ fn test_create_policy() { right: Box::new(Expr::Value(Value::Number("1".parse().unwrap(), false))), }) ); - assert_eq!(with_check, Some(Expr::Value(Value::Boolean(true)))); + assert_eq!( + with_check, + Some(Expr::BinaryOp { + left: Box::new(Expr::Value(Value::Number("1".parse().unwrap(), false))), + op: BinaryOperator::Eq, + right: Box::new(Expr::Value(Value::Number("1".parse().unwrap(), false))), + }) + ); } _ => unreachable!(), } @@ -11208,7 +11213,7 @@ fn test_create_policy() { "AS PERMISSIVE FOR SELECT ", "TO my_role, CURRENT_USER ", "USING (c0 IN (SELECT column FROM t0)) ", - "WITH CHECK (true)" + "WITH CHECK (1 = 1)" )); // omit AS / FOR / TO / USING / WITH CHECK clauses is allowed all_dialects().verified_stmt("CREATE POLICY my_policy ON my_table"); From 759cf09a3c9903184d3d55ae7f48ee700f8d5de7 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 11:28:48 +0100 Subject: [PATCH 3/9] fix parse_alter_table_alter_column test relying on all dialects having a false literal --- tests/sqlparser_common.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 766f473f6..8ed0eae82 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -4113,14 +4113,14 @@ fn parse_alter_table_alter_column() { ); match alter_table_op(verified_stmt(&format!( - "{alter_stmt} ALTER COLUMN is_active SET DEFAULT false" + "{alter_stmt} ALTER COLUMN is_active SET DEFAULT 0" ))) { AlterTableOperation::AlterColumn { column_name, op } => { assert_eq!("is_active", column_name.to_string()); assert_eq!( op, AlterColumnOperation::SetDefault { - value: Expr::Value(Value::Boolean(false)) + value: Expr::Value(Value::Number("0".to_string(), false)) } ); } From effcf5cb700a6d1605be3b45424cf1942887db26 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 11:38:03 +0100 Subject: [PATCH 4/9] The XOR binary operator is specific to MySQL; move its test to mysql-specific tests. --- tests/sqlparser_common.rs | 38 -------------------------------------- tests/sqlparser_mysql.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 8ed0eae82..4054f8f23 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -1919,44 +1919,6 @@ fn parse_binary_all() { ); } -#[test] -fn parse_logical_xor() { - let sql = "SELECT true XOR true, false XOR false, true XOR false, false XOR true"; - let select = verified_only_select(sql); - assert_eq!( - SelectItem::UnnamedExpr(Expr::BinaryOp { - left: Box::new(Expr::Value(Value::Boolean(true))), - op: BinaryOperator::Xor, - right: Box::new(Expr::Value(Value::Boolean(true))), - }), - select.projection[0] - ); - assert_eq!( - SelectItem::UnnamedExpr(Expr::BinaryOp { - left: Box::new(Expr::Value(Value::Boolean(false))), - op: BinaryOperator::Xor, - right: Box::new(Expr::Value(Value::Boolean(false))), - }), - select.projection[1] - ); - assert_eq!( - SelectItem::UnnamedExpr(Expr::BinaryOp { - left: Box::new(Expr::Value(Value::Boolean(true))), - op: BinaryOperator::Xor, - right: Box::new(Expr::Value(Value::Boolean(false))), - }), - select.projection[2] - ); - assert_eq!( - SelectItem::UnnamedExpr(Expr::BinaryOp { - left: Box::new(Expr::Value(Value::Boolean(false))), - op: BinaryOperator::Xor, - right: Box::new(Expr::Value(Value::Boolean(true))), - }), - select.projection[3] - ); -} - #[test] fn parse_between() { fn chk(negated: bool) { diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index 47f7f5b4b..44b2ac6ba 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -2817,3 +2817,42 @@ fn test_group_concat() { mysql_and_generic() .verified_expr("GROUP_CONCAT(DISTINCT test_score ORDER BY test_score DESC SEPARATOR ' ')"); } + +/// The XOR binary operator is only supported in MySQL +#[test] +fn parse_logical_xor() { + let sql = "SELECT true XOR true, false XOR false, true XOR false, false XOR true"; + let select = mysql_and_generic().verified_only_select(sql); + assert_eq!( + SelectItem::UnnamedExpr(Expr::BinaryOp { + left: Box::new(Expr::Value(Value::Boolean(true))), + op: BinaryOperator::Xor, + right: Box::new(Expr::Value(Value::Boolean(true))), + }), + select.projection[0] + ); + assert_eq!( + SelectItem::UnnamedExpr(Expr::BinaryOp { + left: Box::new(Expr::Value(Value::Boolean(false))), + op: BinaryOperator::Xor, + right: Box::new(Expr::Value(Value::Boolean(false))), + }), + select.projection[1] + ); + assert_eq!( + SelectItem::UnnamedExpr(Expr::BinaryOp { + left: Box::new(Expr::Value(Value::Boolean(true))), + op: BinaryOperator::Xor, + right: Box::new(Expr::Value(Value::Boolean(false))), + }), + select.projection[2] + ); + assert_eq!( + SelectItem::UnnamedExpr(Expr::BinaryOp { + left: Box::new(Expr::Value(Value::Boolean(false))), + op: BinaryOperator::Xor, + right: Box::new(Expr::Value(Value::Boolean(true))), + }), + select.projection[3] + ); +} From ee95c427d5cd0048e0f349bbeb39aee0d3997e80 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 12:10:04 +0100 Subject: [PATCH 5/9] fix the parse_merge test not to rely on all dialects supporting boolean litterals --- tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 4054f8f23..594cfdc15 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -8346,7 +8346,7 @@ fn parse_merge() { _ => unreachable!(), }; - let sql = "MERGE INTO s.bar AS dest USING newArrivals AS S ON false WHEN NOT MATCHED THEN INSERT VALUES (stg.A, stg.B, stg.C)"; + let sql = "MERGE INTO s.bar AS dest USING newArrivals AS S ON (1 > 1) WHEN NOT MATCHED THEN INSERT VALUES (stg.A, stg.B, stg.C)"; verified_stmt(sql); } From 0534270ffdb3917c13725ccd9c3c8fb928c666f4 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 12:13:34 +0100 Subject: [PATCH 6/9] fix parse_not_precedence relying on all dialects having boolean literals --- tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 594cfdc15..80aa52e4a 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -1499,7 +1499,7 @@ fn parse_is_not_distinct_from() { #[test] fn parse_not_precedence() { // NOT has higher precedence than OR/AND, so the following must parse as (NOT true) OR true - let sql = "NOT true OR true"; + let sql = "NOT 1 OR 1"; assert_matches!( verified_expr(sql), Expr::BinaryOp { From 82f1ba8d5e8b1aa83b74261e1e0617d9d3c30d08 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 12:16:12 +0100 Subject: [PATCH 7/9] fix parse_top_level test relying on all dialects having a true literal --- tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 80aa52e4a..31ed9508a 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -820,7 +820,7 @@ fn parse_top_level() { verified_stmt("(SELECT 1)"); verified_stmt("((SELECT 1))"); verified_stmt("VALUES (1)"); - verified_stmt("VALUES ROW(1, true, 'a'), ROW(2, false, 'b')"); + verified_stmt("VALUES ROW(1, NULL, 'a'), ROW(2, NULL, 'b')"); } #[test] From 2d25384a2b7045b46c4396ebfbbd859d006dcc99 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Sun, 10 Nov 2024 12:17:28 +0100 Subject: [PATCH 8/9] fix parse_values relying on all dbs having boolean literals --- tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 31ed9508a..90542dc3f 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -6464,7 +6464,7 @@ fn parse_values() { verified_stmt("SELECT * FROM (VALUES (1), (2), (3))"); verified_stmt("SELECT * FROM (VALUES (1), (2), (3)), (VALUES (1, 2, 3))"); verified_stmt("SELECT * FROM (VALUES (1)) UNION VALUES (1)"); - verified_stmt("SELECT * FROM (VALUES ROW(1, true, 'a'), ROW(2, false, 'b')) AS t (a, b, c)"); + verified_stmt("SELECT * FROM (VALUES ROW(1, NULL, 'a'), ROW(2, NULL, 'b')) AS t (a, b, c)"); } #[test] From 1802bfa12586eeb0b7935b6763f9c459dc50e7ce Mon Sep 17 00:00:00 2001 From: lovasoa Date: Wed, 13 Nov 2024 10:14:08 +0100 Subject: [PATCH 9/9] fix tests with --all-features --- tests/sqlparser_common.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 90542dc3f..d413bca23 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -4082,7 +4082,7 @@ fn parse_alter_table_alter_column() { assert_eq!( op, AlterColumnOperation::SetDefault { - value: Expr::Value(Value::Number("0".to_string(), false)) + value: Expr::Value(test_utils::number("0")) } ); } @@ -7291,10 +7291,7 @@ fn lateral_derived() { let join = &from.joins[0]; assert_eq!( join.join_operator, - JoinOperator::LeftOuter(JoinConstraint::On(Expr::Value(Value::Number( - "1".to_string(), - false - )))) + JoinOperator::LeftOuter(JoinConstraint::On(Expr::Value(test_utils::number("1")))) ); if let TableFactor::Derived { lateral,