-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Get expr planners when creating new planner #11485
Changes from all commits
ed18a5b
7cee484
804f77c
47d090a
38b3ff4
ef39772
158569b
9b34d21
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 |
---|---|---|
|
@@ -24,7 +24,6 @@ use arrow_schema::*; | |
use datafusion_common::{ | ||
field_not_found, internal_err, plan_datafusion_err, DFSchemaRef, SchemaError, | ||
}; | ||
use datafusion_expr::planner::ExprPlanner; | ||
use sqlparser::ast::TimezoneInfo; | ||
use sqlparser::ast::{ArrayElemTypeDef, ExactNumberInfo}; | ||
use sqlparser::ast::{ColumnDef as SQLColumnDef, ColumnOption}; | ||
|
@@ -186,8 +185,6 @@ pub struct SqlToRel<'a, S: ContextProvider> { | |
pub(crate) context_provider: &'a S, | ||
pub(crate) options: ParserOptions, | ||
pub(crate) normalizer: IdentNormalizer, | ||
/// user defined planner extensions | ||
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. 👍 |
||
pub(crate) planners: Vec<Arc<dyn ExprPlanner>>, | ||
} | ||
|
||
impl<'a, S: ContextProvider> SqlToRel<'a, S> { | ||
|
@@ -196,12 +193,6 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |
Self::new_with_options(context_provider, ParserOptions::default()) | ||
} | ||
|
||
/// add an user defined planner | ||
pub fn with_user_defined_planner(mut self, planner: Arc<dyn ExprPlanner>) -> Self { | ||
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 we can register planner in session state 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 it makes sense to use the underlying session state as the source of truth |
||
self.planners.push(planner); | ||
self | ||
} | ||
|
||
/// Create a new query planner | ||
pub fn new_with_options(context_provider: &'a S, options: ParserOptions) -> Self { | ||
let normalize = options.enable_ident_normalization; | ||
|
@@ -210,7 +201,6 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |
context_provider, | ||
options, | ||
normalizer: IdentNormalizer::new(normalize), | ||
planners: vec![], | ||
} | ||
} | ||
|
||
|
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 remove
expr_planners
field inSessionContextProvider
and get it from state directlyThere 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.
Nice! I tried to figure out how to do this previously and got confused about a lock or something. This is very nice 👌