iffyio commented on code in PR #2115: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2115#discussion_r2595081777
########## tests/sqlparser_oracle.rs: ########## @@ -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]. + +extern crate core; + +#[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]); + assert_eq!( + &format!("{query}"), + "SELECT ((((3 + 5) || '.3') || 7) - 9) FROM dual" + ); +} + +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(_) => {} + } +} Review 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. ########## tests/sqlparser_oracle.rs: ########## @@ -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]. + +extern crate core; + +#[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]); + assert_eq!( + &format!("{query}"), + "SELECT ((((3 + 5) || '.3') || 7) - 9) FROM dual" + ); +} + +fn oracle_dialect() -> TestedDialects { Review Comment: ```suggestion fn oracle() -> TestedDialects { ``` ########## tests/sqlparser_oracle.rs: ########## @@ -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]. Review Comment: ```suggestion ``` is the clippy attribute necessary here? ########## tests/sqlparser_oracle.rs: ########## @@ -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]. + +extern crate core; Review Comment: what's this used for? ########## src/dialect/oracle.rs: ########## @@ -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, Review 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 ########## src/ast/query.rs: ########## @@ -174,6 +174,15 @@ impl SetExpr { None } } + + /// 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 + } + } Review Comment: ```suggestion ``` 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 ########## tests/sqlparser_oracle.rs: ########## @@ -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]. + +extern crate core; + +#[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]); Review 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? -- 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]
