-
Notifications
You must be signed in to change notification settings - Fork 675
[Oracle] Lower StringConcat precedence #2115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,14 @@ | |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| use super::Dialect; | ||
| use log::debug; | ||
|
|
||
| use crate::{ | ||
| parser::{Parser, ParserError}, | ||
| tokenizer::Token, | ||
| }; | ||
|
|
||
| use super::{Dialect, Precedence}; | ||
|
|
||
| /// A [`Dialect`] for [Oracle Databases](https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlrf/index.html) | ||
| #[derive(Debug)] | ||
|
|
@@ -75,6 +82,35 @@ impl Dialect for OracleDialect { | |
| true | ||
| } | ||
|
|
||
| fn get_next_precedence(&self, _parser: &Parser) -> Option<Result<u8, ParserError>> { | ||
| let t = _parser.peek_token(); | ||
| debug!("get_next_precedence() {t:?}"); | ||
|
|
||
| match t.token { | ||
| Token::StringConcat => Some(Ok(self.prec_value(Precedence::PlusMinus))), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| fn prec_value(&self, prec: Precedence) -> u8 { | ||
| match prec { | ||
| Precedence::Period => 100, | ||
| Precedence::DoubleColon => 50, | ||
| Precedence::AtTz => 41, | ||
| Precedence::MulDivModOp => 40, | ||
| Precedence::PlusMinus => 30, | ||
| Precedence::Xor => 24, | ||
| Precedence::Ampersand => 23, | ||
| Precedence::Caret => 22, | ||
| Precedence::Pipe => 21, | ||
| Precedence::Between | Precedence::Eq | Precedence::Like | Precedence::Is => 20, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| Precedence::PgOther => 16, | ||
| Precedence::UnaryNot => 15, | ||
| Precedence::And => 10, | ||
| Precedence::Or => 5, | ||
| } | ||
| } | ||
|
|
||
| fn supports_group_by_expr(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,81 @@ | ||||||||
| // Licensed to the Apache Software Foundation (ASF) under one | ||||||||
| // or more contributor license agreements. See the NOTICE file | ||||||||
| // distributed with this work for additional information | ||||||||
| // regarding copyright ownership. The ASF licenses this file | ||||||||
| // to you under the Apache License, Version 2.0 (the | ||||||||
| // "License"); you may not use this file except in compliance | ||||||||
| // with the License. You may obtain a copy of the License at | ||||||||
| // | ||||||||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||||||||
| // | ||||||||
| // Unless required by applicable law or agreed to in writing, | ||||||||
| // software distributed under the License is distributed on an | ||||||||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||||||||
| // KIND, either express or implied. See the License for the | ||||||||
| // specific language governing permissions and limitations | ||||||||
| // under the License. | ||||||||
|
|
||||||||
| #![warn(clippy::all)] | ||||||||
| //! Test SQL syntax, specific to [sqlparser::dialect::OracleDialect]. | ||||||||
|
Comment on lines
+17
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
is the clippy attribute necessary here? |
||||||||
| extern crate core; | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this used for? |
||||||||
|
|
||||||||
| #[cfg(test)] | ||||||||
| use pretty_assertions::assert_eq; | ||||||||
|
|
||||||||
| use sqlparser::{ | ||||||||
| ast::{Expr, SelectItem, Value}, | ||||||||
| dialect::OracleDialect, | ||||||||
| }; | ||||||||
| #[cfg(test)] | ||||||||
| use test_utils::TestedDialects; | ||||||||
|
|
||||||||
| mod test_utils; | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn muldiv_have_higher_precedence_than_strconcat() { | ||||||||
| // ~ oracle: `||` has a lower precedence than `*` and `/` | ||||||||
| let sql = "SELECT 3 / 5 || 'asdf' || 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]); | ||||||||
| assert_eq!( | ||||||||
| &format!("{query}"), | ||||||||
| "SELECT (((3 / 5) || 'asdf') || (7 * 9)) FROM dual" | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn plusminus_have_same_precedence_as_strconcat() { | ||||||||
| // ~ 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]); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||||||||
| assert_eq!( | ||||||||
| &format!("{query}"), | ||||||||
| "SELECT ((((3 + 5) || '.3') || 7) - 9) FROM dual" | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| fn oracle_dialect() -> TestedDialects { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| 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(_) => {} | ||||||||
| } | ||||||||
| } | ||||||||
|
Comment on lines
+59
to
+81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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