Skip to content

Commit

Permalink
Upgrade to sqlparser 0.53.0 (apache#13767)
Browse files Browse the repository at this point in the history
* chore: Udpate to sqlparser 0.53.0

* Update for new sqlparser API

* more api updates

* Avoid serializing query to SQL string unless it is necessary

* Box wildcard options

* chore: update datafusion-cli Cargo.lock
  • Loading branch information
alamb authored Dec 20, 2024
1 parent 74480ac commit 75202b5
Show file tree
Hide file tree
Showing 21 changed files with 271 additions and 120 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ recursive = "0.1.1"
regex = "1.8"
rstest = "0.23.0"
serde_json = "1"
sqlparser = { version = "0.52.0", features = ["visitor"] }
sqlparser = { version = "0.53.0", features = ["visitor"] }
tempfile = "3"
tokio = { version = "1.36", features = ["macros", "rt", "sync"] }
url = "2.2"
Expand Down
8 changes: 4 additions & 4 deletions datafusion-cli/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions datafusion/common/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -887,10 +887,10 @@ pub fn get_available_parallelism() -> usize {

#[cfg(test)]
mod tests {
use super::*;
use crate::ScalarValue::Null;
use arrow::array::Float64Array;

use super::*;
use sqlparser::tokenizer::Span;

#[test]
fn test_bisect_linear_left_and_right() -> Result<()> {
Expand Down Expand Up @@ -1118,6 +1118,7 @@ mod tests {
let expected_parsed = vec![Ident {
value: identifier.to_string(),
quote_style,
span: Span::empty(),
}];

assert_eq!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ use arrow_array::{
Array, ArrayRef, Float32Array, Float64Array, Int32Array, RecordBatch, StringArray,
};
use arrow_schema::{DataType, Field, Schema};
use parking_lot::Mutex;
use regex::Regex;
use sqlparser::ast::Ident;

use datafusion::execution::context::{FunctionFactory, RegisterFunction, SessionState};
use datafusion::prelude::*;
use datafusion::{execution::registry::FunctionRegistry, test_util};
Expand All @@ -48,6 +44,10 @@ use datafusion_expr::{
Volatility,
};
use datafusion_functions_nested::range::range_udf;
use parking_lot::Mutex;
use regex::Regex;
use sqlparser::ast::Ident;
use sqlparser::tokenizer::Span;

/// test that casting happens on udfs.
/// c11 is f32, but `custom_sqrt` requires f64. Casting happens but the logical plan and
Expand Down Expand Up @@ -1187,6 +1187,7 @@ async fn create_scalar_function_from_sql_statement_postgres_syntax() -> Result<(
name: Some(Ident {
value: "name".into(),
quote_style: None,
span: Span::empty(),
}),
data_type: DataType::Utf8,
default_expr: None,
Expand All @@ -1196,6 +1197,7 @@ async fn create_scalar_function_from_sql_statement_postgres_syntax() -> Result<(
language: Some(Ident {
value: "plrust".into(),
quote_style: None,
span: Span::empty(),
}),
behavior: None,
function_body: Some(lit(body)),
Expand Down
2 changes: 1 addition & 1 deletion datafusion/expr/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ pub enum Expr {
/// plan into physical plan.
Wildcard {
qualifier: Option<TableReference>,
options: WildcardOptions,
options: Box<WildcardOptions>,
},
/// List of grouping set expressions. Only valid in the context of an aggregate
/// GROUP BY expression list
Expand Down
8 changes: 4 additions & 4 deletions datafusion/expr/src/expr_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,15 @@ pub fn placeholder(id: impl Into<String>) -> Expr {
pub fn wildcard() -> Expr {
Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
options: Box::new(WildcardOptions::default()),
}
}

/// Create an '*' [`Expr::Wildcard`] expression with the wildcard options
pub fn wildcard_with_options(options: WildcardOptions) -> Expr {
Expr::Wildcard {
qualifier: None,
options,
options: Box::new(options),
}
}

Expand All @@ -148,7 +148,7 @@ pub fn wildcard_with_options(options: WildcardOptions) -> Expr {
pub fn qualified_wildcard(qualifier: impl Into<TableReference>) -> Expr {
Expr::Wildcard {
qualifier: Some(qualifier.into()),
options: WildcardOptions::default(),
options: Box::new(WildcardOptions::default()),
}
}

Expand All @@ -159,7 +159,7 @@ pub fn qualified_wildcard_with_options(
) -> Expr {
Expr::Wildcard {
qualifier: Some(qualifier.into()),
options,
options: Box::new(options),
}
}

Expand Down
8 changes: 2 additions & 6 deletions datafusion/optimizer/src/analyzer/inline_table_scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ use crate::analyzer::AnalyzerRule;
use datafusion_common::config::ConfigOptions;
use datafusion_common::tree_node::{Transformed, TransformedResult, TreeNode};
use datafusion_common::{Column, Result};
use datafusion_expr::expr::WildcardOptions;
use datafusion_expr::{logical_plan::LogicalPlan, Expr, LogicalPlanBuilder};
use datafusion_expr::{logical_plan::LogicalPlan, wildcard, Expr, LogicalPlanBuilder};

/// Analyzed rule that inlines TableScan that provide a [`LogicalPlan`]
/// (DataFrame / ViewTable)
Expand Down Expand Up @@ -93,10 +92,7 @@ fn generate_projection_expr(
)));
}
} else {
exprs.push(Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
});
exprs.push(wildcard());
}
Ok(exprs)
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/proto/src/logical_plan/from_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ pub fn parse_expr(
let qualifier = qualifier.to_owned().map(|x| x.try_into()).transpose()?;
Ok(Expr::Wildcard {
qualifier,
options: WildcardOptions::default(),
options: Box::new(WildcardOptions::default()),
})
}
ExprType::ScalarUdfExpr(protobuf::ScalarUdfExprNode {
Expand Down
12 changes: 3 additions & 9 deletions datafusion/proto/tests/cases/roundtrip_logical_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ use datafusion_common::{
use datafusion_expr::dml::CopyTo;
use datafusion_expr::expr::{
self, Between, BinaryExpr, Case, Cast, GroupingSet, InList, Like, ScalarFunction,
Unnest, WildcardOptions,
Unnest,
};
use datafusion_expr::logical_plan::{Extension, UserDefinedLogicalNodeCore};
use datafusion_expr::{
Expand Down Expand Up @@ -2061,21 +2061,15 @@ fn roundtrip_unnest() {

#[test]
fn roundtrip_wildcard() {
let test_expr = Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
};
let test_expr = wildcard();

let ctx = SessionContext::new();
roundtrip_expr_test(test_expr, ctx);
}

#[test]
fn roundtrip_qualified_wildcard() {
let test_expr = Expr::Wildcard {
qualifier: Some("foo".into()),
options: WildcardOptions::default(),
};
let test_expr = qualified_wildcard("foo");

let ctx = SessionContext::new();
roundtrip_expr_test(test_expr, ctx);
Expand Down
24 changes: 10 additions & 14 deletions datafusion/sql/src/expr/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ use datafusion_common::{
internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err,
DFSchema, Dependency, Result,
};
use datafusion_expr::expr::WildcardOptions;
use datafusion_expr::expr::{ScalarFunction, Unnest};
use datafusion_expr::planner::PlannerResult;
use datafusion_expr::{
expr, Expr, ExprFunctionExt, ExprSchemable, WindowFrame, WindowFunctionDefinition,
expr, qualified_wildcard, wildcard, Expr, ExprFunctionExt, ExprSchemable,
WindowFrame, WindowFunctionDefinition,
};
use sqlparser::ast::{
DuplicateTreatment, Expr as SQLExpr, Function as SQLFunction, FunctionArg,
Expand Down Expand Up @@ -169,6 +169,11 @@ impl FunctionArgs {
"Calling {name}: SEPARATOR not supported in function arguments: {sep}"
)
}
FunctionArgumentClause::JsonNullClause(jn) => {
return not_impl_err!(
"Calling {name}: JSON NULL clause not supported in function arguments: {jn}"
)
}
}
}

Expand Down Expand Up @@ -413,28 +418,19 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
name: _,
arg: FunctionArgExpr::Wildcard,
operator: _,
} => Ok(Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
}),
} => Ok(wildcard()),
FunctionArg::Unnamed(FunctionArgExpr::Expr(arg)) => {
self.sql_expr_to_logical_expr(arg, schema, planner_context)
}
FunctionArg::Unnamed(FunctionArgExpr::Wildcard) => Ok(Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
}),
FunctionArg::Unnamed(FunctionArgExpr::Wildcard) => Ok(wildcard()),
FunctionArg::Unnamed(FunctionArgExpr::QualifiedWildcard(object_name)) => {
let qualifier = self.object_name_to_table_reference(object_name)?;
// Sanity check on qualifier with schema
let qualified_indices = schema.fields_indices_with_qualified(&qualifier);
if qualified_indices.is_empty() {
return plan_err!("Invalid qualifier {qualifier}");
}
Ok(Expr::Wildcard {
qualifier: Some(qualifier),
options: WildcardOptions::default(),
})
Ok(qualified_wildcard(qualifier))
}
_ => not_impl_err!("Unsupported qualified wildcard argument: {sql:?}"),
}
Expand Down
8 changes: 4 additions & 4 deletions datafusion/sql/src/expr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,13 +593,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
}
not_impl_err!("AnyOp not supported by ExprPlanner: {binary_expr:?}")
}
SQLExpr::Wildcard => Ok(Expr::Wildcard {
SQLExpr::Wildcard(_token) => Ok(Expr::Wildcard {
qualifier: None,
options: WildcardOptions::default(),
options: Box::new(WildcardOptions::default()),
}),
SQLExpr::QualifiedWildcard(object_name) => Ok(Expr::Wildcard {
SQLExpr::QualifiedWildcard(object_name, _token) => Ok(Expr::Wildcard {
qualifier: Some(self.object_name_to_table_reference(object_name)?),
options: WildcardOptions::default(),
options: Box::new(WildcardOptions::default()),
}),
SQLExpr::Tuple(values) => self.parse_tuple(schema, planner_context, values),
_ => not_impl_err!("Unsupported ast node in sqltorel: {sql:?}"),
Expand Down
14 changes: 12 additions & 2 deletions datafusion/sql/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ use std::collections::VecDeque;
use std::fmt;

use sqlparser::ast::ExprWithAlias;
use sqlparser::tokenizer::TokenWithSpan;
use sqlparser::{
ast::{
ColumnDef, ColumnOptionDef, ObjectName, OrderByExpr, Query,
Statement as SQLStatement, TableConstraint, Value,
},
dialect::{keywords::Keyword, Dialect, GenericDialect},
parser::{Parser, ParserError},
tokenizer::{Token, TokenWithLocation, Tokenizer, Word},
tokenizer::{Token, Tokenizer, Word},
};

// Use `Parser::expected` instead, if possible
Expand Down Expand Up @@ -338,7 +339,7 @@ impl<'a> DFParser<'a> {
fn expected<T>(
&self,
expected: &str,
found: TokenWithLocation,
found: TokenWithSpan,
) -> Result<T, ParserError> {
parser_err!(format!("Expected {expected}, found: {found}"))
}
Expand Down Expand Up @@ -876,6 +877,7 @@ mod tests {
use super::*;
use sqlparser::ast::Expr::Identifier;
use sqlparser::ast::{BinaryOperator, DataType, Expr, Ident};
use sqlparser::tokenizer::Span;

fn expect_parse_ok(sql: &str, expected: Statement) -> Result<(), ParserError> {
let statements = DFParser::parse_sql(sql)?;
Expand Down Expand Up @@ -911,6 +913,7 @@ mod tests {
name: Ident {
value: name.into(),
quote_style: None,
span: Span::empty(),
},
data_type,
collation: None,
Expand Down Expand Up @@ -1219,6 +1222,7 @@ mod tests {
expr: Identifier(Ident {
value: "c1".to_owned(),
quote_style: None,
span: Span::empty(),
}),
asc,
nulls_first,
Expand Down Expand Up @@ -1250,6 +1254,7 @@ mod tests {
expr: Identifier(Ident {
value: "c1".to_owned(),
quote_style: None,
span: Span::empty(),
}),
asc: Some(true),
nulls_first: None,
Expand All @@ -1259,6 +1264,7 @@ mod tests {
expr: Identifier(Ident {
value: "c2".to_owned(),
quote_style: None,
span: Span::empty(),
}),
asc: Some(false),
nulls_first: Some(true),
Expand Down Expand Up @@ -1290,11 +1296,13 @@ mod tests {
left: Box::new(Identifier(Ident {
value: "c1".to_owned(),
quote_style: None,
span: Span::empty(),
})),
op: BinaryOperator::Minus,
right: Box::new(Identifier(Ident {
value: "c2".to_owned(),
quote_style: None,
span: Span::empty(),
})),
},
asc: Some(true),
Expand Down Expand Up @@ -1335,11 +1343,13 @@ mod tests {
left: Box::new(Identifier(Ident {
value: "c1".to_owned(),
quote_style: None,
span: Span::empty(),
})),
op: BinaryOperator::Minus,
right: Box::new(Identifier(Ident {
value: "c2".to_owned(),
quote_style: None,
span: Span::empty(),
})),
},
asc: Some(true),
Expand Down
Loading

0 comments on commit 75202b5

Please sign in to comment.