Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/ast/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Comment on lines +177 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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

}

impl fmt::Display for SetExpr {
Expand Down
38 changes: 37 additions & 1 deletion src/dialect/oracle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
Expand Down
81 changes: 81 additions & 0 deletions tests/sqlparser_oracle.rs
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#![warn(clippy::all)]
//! Test SQL syntax, specific to [sqlparser::dialect::OracleDialect].

is the clippy attribute necessary here?

extern crate core;
Copy link
Contributor

Choose a reason for hiding this comment

The 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]);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn oracle_dialect() -> TestedDialects {
fn oracle() -> 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(_) => {}
}
}
Comment on lines +59 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The 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.