iffyio commented on code in PR #1472:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1472#discussion_r1824161376
##########
tests/sqlparser_common.rs:
##########
@@ -11460,3 +11460,54 @@ 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() {
Review Comment:
since this MR also introduces the `supports_factorial_operator` dialect
method, can we add a similar test case for that one as well?
##########
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:
Ah so both dialects support that syntax but not to mean the same thing
(hence the dialect flags), the hive syntax is only a prefix operator so that I
expect we wouldn't need any code at this stage (`parse_infix()`) for it (if we
do then we can demonstrate that with a test case that fails to parse a valid
expression without the block, but I didn't think there should be such a case)?
So that the block `} else if Token::ExclamationMark == tok &&
self.dialect.supports_bang_not_operator() {` doesn't seem to have any effect
and ideally should be removed if so.
##########
tests/sqlparser_common.rs:
##########
@@ -11460,3 +11460,54 @@ 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 [
Review Comment:
the `op` variable seems redundant given its always set to
`UnaryOperator::BangNot`? so that the loop can probably be simplified to `for
(i, expr) in [...]`
--
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
