iffyio commented on code in PR #1472:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1472#discussion_r1821869845
##########
src/dialect/mod.rs:
##########
@@ -590,6 +595,11 @@ pub trait Dialect: Debug + Any {
fn supports_try_convert(&self) -> bool {
false
}
+
+ /// Returns true if the dialect supports `!a` expressions
Review Comment:
```suggestion
/// Returns true if the dialect supports `!a` syntax for boolean `NOT`
expressions.
```
##########
src/dialect/hive.rs:
##########
@@ -51,4 +51,8 @@ impl Dialect for HiveDialect {
fn require_interval_qualifier(&self) -> bool {
true
}
+
+ fn supports_bang_not_operator(&self) -> bool {
Review Comment:
Do we have a link to the syntax? If so it would be good to include that here
as part of the documentation
##########
tests/sqlparser_common.rs:
##########
@@ -11460,3 +11460,49 @@ fn test_try_convert() {
all_dialects_where(|d| d.supports_try_convert() &&
!d.convert_type_before_value());
dialects.verified_expr("TRY_CONVERT('foo', VARCHAR(MAX))");
}
+
+#[test]
+fn parse_bang_not() {
+ let dialects = all_dialects_where(|d| d.supports_bang_not_operator());
+ let sql = "SELECT !a, !(b > 3)";
+ let Select { projection, .. } = dialects.verified_only_select(sql);
+
+ for (i, (op, expr)) in [
+ (
+ UnaryOperator::BangNot,
+ Box::new(Expr::Identifier(Ident::new("a"))),
+ ),
+ (
+ UnaryOperator::BangNot,
+ Box::new(Expr::Nested(Box::new(Expr::BinaryOp {
+ left: Box::new(Expr::Identifier(Ident::new("b"))),
+ op: BinaryOperator::Gt,
+ right:
Box::new(Expr::Value(Value::Number("3".parse().unwrap(), false))),
+ }))),
+ ),
+ ]
+ .into_iter()
+ .enumerate()
+ {
+ assert_eq!(
+ SelectItem::UnnamedExpr(Expr::UnaryOp { op: op, expr }),
+ projection[i]
+ )
+ }
+
+ let sql = "SELECT a!";
Review Comment:
Can we add a similar test case for `SELECT a ! b` to see how the parser
behaves?
##########
src/parser/mod.rs:
##########
@@ -2028,6 +2035,13 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_bang_not(&mut self) -> Result<Expr, ParserError> {
+ Ok(Expr::UnaryOp {
+ op: UnaryOperator::BangNot,
+ expr:
Box::new(self.parse_subexpr(self.dialect.prec_value(Precedence::UnaryNot))?),
+ })
+ }
Review Comment:
this one is unused and we can remove?
##########
src/parser/mod.rs:
##########
@@ -2800,11 +2814,40 @@ impl<'a> Parser<'a> {
format: None,
})
} else if Token::ExclamationMark == tok {
- // PostgreSQL factorial operation
- Ok(Expr::UnaryOp {
- op: UnaryOperator::PGPostfixFactorial,
- expr: Box::new(expr),
- })
+ if self.dialect.supports_factorial_operator() {
+ match expr {
+ Expr::Value(_) | Expr::Identifier(_) | Expr::Nested(_) |
Expr::BinaryOp{..} => Ok(Expr::UnaryOp {
+ op: UnaryOperator::PGPostfixFactorial,
+ expr: Box::new(expr),
+ }),
+ _ => {
+ self.expected(
+ "Value or Identifier or Nested or BinaryOp struct
before factorial operator(!)", self.peek_token())
Review Comment:
hmm why do we make this change, is the current behavior to accept arbitrary
expression seems more reasonable otherwise?
##########
src/parser/mod.rs:
##########
@@ -2800,11 +2814,40 @@ impl<'a> Parser<'a> {
format: None,
})
} else if Token::ExclamationMark == tok {
- // PostgreSQL factorial operation
- Ok(Expr::UnaryOp {
- op: UnaryOperator::PGPostfixFactorial,
- expr: Box::new(expr),
- })
+ if self.dialect.supports_factorial_operator() {
Review Comment:
I think we can avoid the extra nesting (and having to handle the invalid
case explicitly) by keeping each feature as its own if block
```rust
else if Token::ExclamationMark == tok &&
self.dialect.supports_factorial_operator() {
...
} else if Token::Exclamationmark == tok &&
self.dialect.supports_bang_not_operator() {
...
}
```
##########
src/parser/mod.rs:
##########
@@ -2800,11 +2814,40 @@ impl<'a> Parser<'a> {
format: None,
})
} else if Token::ExclamationMark == tok {
- // PostgreSQL factorial operation
- Ok(Expr::UnaryOp {
- op: UnaryOperator::PGPostfixFactorial,
- expr: Box::new(expr),
- })
+ if self.dialect.supports_factorial_operator() {
+ match expr {
+ Expr::Value(_) | Expr::Identifier(_) | Expr::Nested(_) |
Expr::BinaryOp{..} => Ok(Expr::UnaryOp {
+ op: UnaryOperator::PGPostfixFactorial,
+ expr: Box::new(expr),
+ }),
+ _ => {
+ self.expected(
+ "Value or Identifier or Nested or BinaryOp struct
before factorial operator(!)", self.peek_token())
+ },
+ }
+ } else if self.dialect.supports_bang_not_operator() {
Review Comment:
Oh actually I'm wondering if this part is needed at all (expecting that this
operator is only a prefix operator). Are there test cases that fail without
this block?
--
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
