LucaCappelletti94 commented on code in PR #2829:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2829#discussion_r4484814492
##########
src/dialect/mod.rs:
##########
@@ -1185,6 +1185,11 @@ pub trait Dialect: Debug + Any {
self.prec_value(Precedence::Between)
}
+ /// Returns the precedence used to parse the operand of unary `+` and `-`.
+ fn unary_plus_minus_precedence(&self) -> u8 {
+ self.prec_value(Precedence::MulDivModOp)
+ }
+
Review Comment:
You could remove this method and bound the operand through `prec_value` with
a new `Precedence::UnaryPlusMinus`, as the other prefix operators in
`parse_prefix` already do (`src/parser/mod.rs:2025`, `2047`, `2053`, `2077`).
```suggestion
```
##########
src/dialect/mod.rs:
##########
Review Comment:
You could map the new variant to 40 here, which keeps the current bound for
every dialect on the default table.
```suggestion
Precedence::MulDivModOp | Precedence::UnaryPlusMinus => 40,
```
##########
src/parser/mod.rs:
##########
@@ -2017,9 +2017,7 @@ impl<'a> Parser<'a> {
};
Ok(Expr::UnaryOp {
op,
- expr: Box::new(
-
self.parse_subexpr(self.dialect.prec_value(Precedence::MulDivModOp))?,
- ),
+ expr:
Box::new(self.parse_subexpr(self.dialect.unary_plus_minus_precedence())?),
Review Comment:
You could read the bound from the dialect table.
```suggestion
expr: Box::new(
self.parse_subexpr(self.dialect.prec_value(Precedence::UnaryPlusMinus))?,
),
```
##########
src/dialect/postgresql.rs:
##########
Review Comment:
You could map the variant to the new level here.
```suggestion
Precedence::AtTz => AT_TZ_PREC,
Precedence::UnaryPlusMinus => UNARY_PLUS_MINUS_PREC,
```
##########
src/dialect/postgresql.rs:
##########
Review Comment:
You should put the unary level between `BRACKET_PREC` and `COLLATE_PREC`,
because `Caret` is still too low. In
[gram.y](https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y)
`UMINUS` sits above `AT` and `COLLATE` and below `[` and `::`. PostgreSQL 17
`EXPLAIN VERBOSE` groups `- ts AT TIME ZONE 'UTC'` as `(- ts) AT TIME ZONE
'UTC'` with a user-defined prefix `-` on `timestamp`, and this head parses
`-(ts AT TIME ZONE 'UTC')`.
```suggestion
const BRACKET_PREC: u8 = 130;
const UNARY_PLUS_MINUS_PREC: u8 = 125;
```
##########
src/dialect/mod.rs:
##########
Review Comment:
You could add the variant after `AtTz`.
```suggestion
AtTz,
/// Unary `+` and `-`, bounding their operand.
UnaryPlusMinus,
```
##########
src/dialect/postgresql.rs:
##########
@@ -390,6 +390,10 @@ impl Dialect for PostgreSqlDialect {
}
}
+ fn unary_plus_minus_precedence(&self) -> u8 {
+ self.prec_value(Precedence::Caret)
+ }
+
Review Comment:
You could drop this override once `prec_value` carries the PostgreSQL level.
```suggestion
```
##########
tests/sqlparser_postgres.rs:
##########
@@ -13154,3 +13154,51 @@ fn parse_alter_table_no_inherit() {
);
}
}
+
+#[test]
+fn parse_unary_plus_minus_before_pg_exponentiation() {
+ for (sql, unary_op, operand, exponent) in [
+ ("SELECT -1 ^ 0", UnaryOperator::Minus, "1", "0"),
+ ("SELECT +2 ^ 2", UnaryOperator::Plus, "2", "2"),
+ ] {
+ let select = pg().verified_only_select(sql);
+ let Expr::BinaryOp { left, op, right } =
expr_from_projection(&select.projection[0]) else {
+ panic!("Expected exponentiation expression")
+ };
+ assert_eq!(*op, BinaryOperator::PGExp);
+ assert_eq!(
+ right.as_ref(),
+ &Expr::Value(number(exponent).with_empty_span())
+ );
+ assert_eq!(
+ left.as_ref(),
+ &Expr::UnaryOp {
+ op: unary_op,
+ expr: Box::new(Expr::Value(number(operand).with_empty_span())),
+ }
+ );
+ }
+}
Review Comment:
You could cover the `AT TIME ZONE` grouping as well. On this head the test
panics with `Expected AT TIME ZONE expression`.
```suggestion
}
#[test]
fn parse_unary_minus_before_pg_at_time_zone() {
let select = pg().verified_only_select("SELECT -ts AT TIME ZONE 'UTC'");
let Expr::AtTimeZone { timestamp, .. } =
expr_from_projection(&select.projection[0]) else {
panic!("Expected AT TIME ZONE expression")
};
assert_eq!(
timestamp.as_ref(),
&Expr::UnaryOp {
op: UnaryOperator::Minus,
expr: Box::new(Expr::Identifier(Ident::new("ts"))),
}
);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
