-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor: add dialect enum #18043
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
refactor: add dialect enum #18043
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,15 +30,14 @@ use crate::datasource::provider_as_source; | |
| use crate::execution::context::{EmptySerializerRegistry, FunctionFactory, QueryPlanner}; | ||
| use crate::execution::SessionStateDefaults; | ||
| use crate::physical_planner::{DefaultPhysicalPlanner, PhysicalPlanner}; | ||
| use arrow::datatypes::DataType; | ||
| use datafusion_catalog::information_schema::{ | ||
| InformationSchemaProvider, INFORMATION_SCHEMA, | ||
| }; | ||
|
|
||
| use arrow::datatypes::DataType; | ||
| use datafusion_catalog::MemoryCatalogProviderList; | ||
| use datafusion_catalog::{TableFunction, TableFunctionImpl}; | ||
| use datafusion_common::alias::AliasGenerator; | ||
| use datafusion_common::config::{ConfigExtension, ConfigOptions, TableOptions}; | ||
| use datafusion_common::config::{ConfigExtension, ConfigOptions, Dialect, TableOptions}; | ||
| use datafusion_common::display::{PlanType, StringifiedPlan, ToStringifiedPlan}; | ||
| use datafusion_common::tree_node::TreeNode; | ||
| use datafusion_common::{ | ||
|
|
@@ -374,7 +373,7 @@ impl SessionState { | |
| pub fn sql_to_statement( | ||
|
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 this is technically a breaking API change (now it requires a &Dialect). Maybe can you add a note to the upgrade guide and a doc example of how to use this API to help people upgrade? This is explained a bit more in https://datafusion.apache.org/contributor-guide/api-health.html#upgrade-guides
Contributor
Author
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 added a section in Should I add it something more? |
||
| &self, | ||
| sql: &str, | ||
| dialect: &str, | ||
| dialect: &Dialect, | ||
| ) -> datafusion_common::Result<Statement> { | ||
| let dialect = dialect_from_str(dialect).ok_or_else(|| { | ||
| plan_datafusion_err!( | ||
|
|
@@ -411,7 +410,7 @@ impl SessionState { | |
| pub fn sql_to_expr( | ||
| &self, | ||
| sql: &str, | ||
| dialect: &str, | ||
| dialect: &Dialect, | ||
| ) -> datafusion_common::Result<SQLExpr> { | ||
| self.sql_to_expr_with_alias(sql, dialect).map(|x| x.expr) | ||
| } | ||
|
|
@@ -423,7 +422,7 @@ impl SessionState { | |
| pub fn sql_to_expr_with_alias( | ||
| &self, | ||
| sql: &str, | ||
| dialect: &str, | ||
| dialect: &Dialect, | ||
| ) -> datafusion_common::Result<SQLExprWithAlias> { | ||
| let dialect = dialect_from_str(dialect).ok_or_else(|| { | ||
| plan_datafusion_err!( | ||
|
|
@@ -527,8 +526,8 @@ impl SessionState { | |
| &self, | ||
| sql: &str, | ||
| ) -> datafusion_common::Result<LogicalPlan> { | ||
| let dialect = self.config.options().sql_parser.dialect.as_str(); | ||
| let statement = self.sql_to_statement(sql, dialect)?; | ||
| let dialect = self.config.options().sql_parser.dialect; | ||
| let statement = self.sql_to_statement(sql, &dialect)?; | ||
| let plan = self.statement_to_plan(statement).await?; | ||
| Ok(plan) | ||
| } | ||
|
|
@@ -542,9 +541,9 @@ impl SessionState { | |
| sql: &str, | ||
| df_schema: &DFSchema, | ||
| ) -> datafusion_common::Result<Expr> { | ||
| let dialect = self.config.options().sql_parser.dialect.as_str(); | ||
| let dialect = self.config.options().sql_parser.dialect; | ||
|
|
||
| let sql_expr = self.sql_to_expr_with_alias(sql, dialect)?; | ||
| let sql_expr = self.sql_to_expr_with_alias(sql, &dialect)?; | ||
|
|
||
| let provider = SessionContextProvider { | ||
| state: self, | ||
|
|
@@ -2034,6 +2033,7 @@ mod tests { | |
| use arrow::array::{ArrayRef, Int32Array, RecordBatch, StringArray}; | ||
| use arrow::datatypes::{DataType, Field, Schema}; | ||
| use datafusion_catalog::MemoryCatalogProviderList; | ||
| use datafusion_common::config::Dialect; | ||
| use datafusion_common::DFSchema; | ||
| use datafusion_common::Result; | ||
| use datafusion_execution::config::SessionConfig; | ||
|
|
@@ -2059,8 +2059,8 @@ mod tests { | |
| let sql = "[1,2,3]"; | ||
| let schema = Schema::new(vec![Field::new("a", DataType::Int32, true)]); | ||
| let df_schema = DFSchema::try_from(schema)?; | ||
| let dialect = state.config.options().sql_parser.dialect.as_str(); | ||
| let sql_expr = state.sql_to_expr(sql, dialect)?; | ||
| let dialect = state.config.options().sql_parser.dialect; | ||
| let sql_expr = state.sql_to_expr(sql, &dialect)?; | ||
|
|
||
| let query = SqlToRel::new_with_options(&provider, state.get_parser_options()); | ||
| query.sql_to_expr(sql_expr, &df_schema, &mut PlannerContext::new()) | ||
|
|
@@ -2218,7 +2218,8 @@ mod tests { | |
| } | ||
|
|
||
| let state = &context_provider.state; | ||
| let statement = state.sql_to_statement("select count(*) from t", "mysql")?; | ||
| let statement = | ||
| state.sql_to_statement("select count(*) from t", &Dialect::MySQL)?; | ||
| let plan = SqlToRel::new(&context_provider).statement_to_plan(statement)?; | ||
| state.create_physical_plan(&plan).await | ||
| } | ||
|
|
||
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.
Can you please add documentation to this enum that explains:
Specifically it would help to understand if the intent is to mirror the code in
sqlparser(but is replicated to avoid adding a sqlparser dependency)Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you for your feedback, I added documentation