[Oracle] Lower StringConcat precedence #2115
Conversation
xitep
force-pushed
the
oracle-dialect-operator-precedence
branch
4 times, most recently
from
1307704 to
d57daaf
Compare
xitep
force-pushed
the
oracle-dialect-operator-precedence
branch
from
d57daaf to
1914d72
Compare
| fn oracle_dialect() -> TestedDialects { | ||
| TestedDialects::new(vec![Box::new(OracleDialect)]) | ||
| } | ||
|
|
||
| /// Wraps [Expr::BinaryExpr]s in `item` with a [Expr::Nested] recursively. | ||
| fn nest_binary_ops(item: &mut SelectItem) { | ||
| // ~ idealy, we could use `VisitorMut` at this point | ||
| fn nest(expr: &mut Expr) { | ||
| // ~ ideally we could use VisitorMut here | ||
| if let Expr::BinaryOp { left, op: _, right } = expr { | ||
| nest(&mut *left); | ||
| nest(&mut *right); | ||
| let inner = std::mem::replace(expr, Expr::Value(Value::Null.into())); | ||
| *expr = Expr::Nested(Box::new(inner)); | ||
| } | ||
| } | ||
| match item { | ||
| SelectItem::UnnamedExpr(expr) => nest(expr), | ||
| SelectItem::ExprWithAlias { expr, alias: _ } => nest(expr), | ||
| SelectItem::QualifiedWildcard(_, _) => {} | ||
| SelectItem::Wildcard(_) => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
can we move these to the top of the file instead? folks tend to add new tests to the bottom of the file, so that these don't end up awkwardly in the middle over time.
| ); | ||
| } | ||
|
|
||
| fn oracle_dialect() -> TestedDialects { |
There was a problem hiding this comment.
| fn oracle_dialect() -> TestedDialects { | |
| fn oracle() -> TestedDialects { |
| #![warn(clippy::all)] | ||
| //! Test SQL syntax, specific to [sqlparser::dialect::OracleDialect]. | ||
| extern crate core; |
|
|
||
| #![warn(clippy::all)] | ||
| //! Test SQL syntax, specific to [sqlparser::dialect::OracleDialect]. |
There was a problem hiding this comment.
| #![warn(clippy::all)] | |
| //! Test SQL syntax, specific to [sqlparser::dialect::OracleDialect]. |
is the clippy attribute necessary here?
|
|
||
| /// If this `SetExpr` is a `SELECT`, returns a mutable [`Select`]. | ||
| pub fn as_select_mut(&mut self) -> Option<&mut Select> { | ||
| if let Self::Select(select) = self { | ||
| Some(&mut **select) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
| /// If this `SetExpr` is a `SELECT`, returns a mutable [`Select`]. | |
| pub fn as_select_mut(&mut self) -> Option<&mut Select> { | |
| if let Self::Select(select) = self { | |
| Some(&mut **select) | |
| } else { | |
| None | |
| } | |
| } |
I think we would want to avoid a public mut API that is only used for internal tests, if needed it would likely make more sense as a helper function in the test module
| // ~ oracle: `+`, `-`, and `||` have the same precedence and parse from left-to-right | ||
| let sql = "SELECT 3 + 5 || '.3' || 7 - 9 FROM dual"; | ||
| let mut query = oracle_dialect().verified_query(sql); | ||
| nest_binary_ops(&mut query.body.as_select_mut().expect("not a SELECT").projection[0]); |
There was a problem hiding this comment.
I think instead of introducing the nest_binary_ops tfn hat mutates the ast, can we instead explicitly assert what we expect the ast to look like?
| Precedence::Ampersand => 23, | ||
| Precedence::Caret => 22, | ||
| Precedence::Pipe => 21, | ||
| Precedence::Between | Precedence::Eq | Precedence::Like | Precedence::Is => 20, |
There was a problem hiding this comment.
This looks like the only changes compared to the default dialect precedence, can we add tests demonstrating what they imply?
Also since we're manually overriding all the precedence values, it would be good to have test coverage for each variant here for oracle as well
2 participants
No description provided.