Get expr planners when creating new planner #11485

jayzhan211 · 2024-07-16T00:28:22Z

Which issue does this PR close?

Closes #11477 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-07-16T01:26:07Z

We may even remove planners in SqlToRel 🤔 ?

/// SQL query planner
pub struct SqlToRel<'a, S: ContextProvider> {
    pub(crate) context_provider: &'a S,
    pub(crate) options: ParserOptions,
    pub(crate) normalizer: IdentNormalizer,
    /// user defined planner extensions
    pub(crate) planners: Vec<Arc<dyn ExprPlanner>>,
}

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-07-16T01:30:20Z

datafusion/sql/src/planner.rs

@@ -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 {


I think we can register planner in session state

I think it makes sense to use the underlying session state as the source of truth

alamb

Thanks @jayzhan211 this makes sense to me.

I have a suggestion to avoid creating Vec's for all accesses to extension planners. Let me know what you think: jayzhan211#3

I also wonder if we should add a test case to avoid this kind of issue from happening again (whatever API Lance and Delta.rs are using clearly isn't covered by our existing tests)

cc @wjones127 and @rtyler (who also mentioned this #11180 (comment))

alamb · 2024-07-16T20:59:10Z

datafusion/sql/src/planner.rs

@@ -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


alamb · 2024-07-16T20:59:28Z

datafusion/sql/src/planner.rs

@@ -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 {


I think it makes sense to use the underlying session state as the source of truth

alamb · 2024-07-16T21:18:46Z

datafusion/expr/src/planner.rs

@@ -60,6 +60,9 @@ pub trait ContextProvider {
        not_impl_err!("Recursive CTE is not implemented")
    }

+    /// Getter for expr planners
+    fn get_expr_planners(&self) -> Vec<Arc<dyn ExprPlanner>>;


I spent some time trying to figure out if we could avoid this Vec cloning all the time -- with

Suggested change

fn get_expr_planners(&self) -> Vec<Arc<dyn ExprPlanner>>;

fn get_expr_planners(&self) -> &[Arc<dyn ExprPlanner>] {&[]}

I think this is possible -- I made jayzhan211#3 to show how it looks

wjones127

Thanks. This seems like a good simplification. I also agree that we could make get_expr_planners return a slice.

Use Slice rather than Vec to access expr planners

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-07-17T00:51:39Z

datafusion/core/src/execution/session_state.rs

 struct SessionContextProvider<'a> {
    state: &'a SessionState,
    tables: HashMap<String, Arc<dyn TableSource>>,
 }

 impl<'a> ContextProvider for SessionContextProvider<'a> {
+    fn get_expr_planners(&self) -> &[Arc<dyn ExprPlanner>] {
+        &self.state.expr_planners


I remove expr_planners field in SessionContextProvider and get it from state directly

Nice! I tried to figure out how to do this previously and got confused about a lock or something. This is very nice 👌

Signed-off-by: jayzhan211 <[email protected]>

alamb · 2024-07-17T10:29:31Z

datafusion/core/src/execution/session_state.rs

 struct SessionContextProvider<'a> {
    state: &'a SessionState,
    tables: HashMap<String, Arc<dyn TableSource>>,
 }

 impl<'a> ContextProvider for SessionContextProvider<'a> {
+    fn get_expr_planners(&self) -> &[Arc<dyn ExprPlanner>] {
+        &self.state.expr_planners


Nice! I tried to figure out how to do this previously and got confused about a lock or something. This is very nice 👌

alamb · 2024-07-17T10:30:04Z

Thanks @jayzhan211 and @wjones127

* get expr planners when creating new planner Signed-off-by: jayzhan211 <[email protected]> * get expr planner when creating planner Signed-off-by: jayzhan211 <[email protected]> * no planners in sqltorel Signed-off-by: jayzhan211 <[email protected]> * Add docs about SessionContextProvider * Use Slice rather than Vec to access expr planners * add test Signed-off-by: jayzhan211 <[email protected]> * clippy Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

get expr planners when creating new planner

ed18a5b

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate labels Jul 16, 2024

get expr planner when creating planner

7cee484

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added the optimizer Optimizer rules label Jul 16, 2024

jayzhan211 marked this pull request as ready for review July 16, 2024 01:16

jayzhan211 marked this pull request as draft July 16, 2024 01:20

jayzhan211 marked this pull request as ready for review July 16, 2024 01:23

no planners in sqltorel

804f77c

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 commented Jul 16, 2024

View reviewed changes

Add docs about SessionContextProvider

47d090a

alamb mentioned this pull request Jul 16, 2024

Use Slice rather than Vec to access expr planners jayzhan211/datafusion#3

Merged

Use Slice rather than Vec to access expr planners

38b3ff4

alamb approved these changes Jul 16, 2024

View reviewed changes

alamb mentioned this pull request Jul 16, 2024

Introduce user defined SQL planner API #11180

Merged

wjones127 reviewed Jul 16, 2024

View reviewed changes

Merge pull request #3 from alamb/alamb/use_slice

ef39772

Use Slice rather than Vec to access expr planners

github-actions bot removed the optimizer Optimizer rules label Jul 17, 2024

add test

158569b

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 commented Jul 17, 2024

View reviewed changes

clippy

9b34d21

Signed-off-by: jayzhan211 <[email protected]>

alamb approved these changes Jul 17, 2024

View reviewed changes

alamb merged commit de0765a into apache:main Jul 17, 2024
23 checks passed

dharanad mentioned this pull request Jul 18, 2024

Move sql_compound_identifier_to_expr to ExprPlanner #11487

Merged

alexwilcoxson-rel mentioned this pull request Sep 20, 2024

build(deps): datafusion 41 lancedb/lance#2917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get expr planners when creating new planner #11485

Get expr planners when creating new planner #11485

jayzhan211 commented Jul 16, 2024

jayzhan211 commented Jul 16, 2024

jayzhan211 Jul 16, 2024

alamb Jul 16, 2024

alamb left a comment

alamb Jul 16, 2024

alamb Jul 16, 2024

alamb Jul 16, 2024

wjones127 left a comment

jayzhan211 Jul 17, 2024

alamb Jul 17, 2024

alamb Jul 17, 2024

alamb commented Jul 17, 2024

	fn get_expr_planners(&self) -> Vec<Arc<dyn ExprPlanner>>;
	fn get_expr_planners(&self) -> &[Arc<dyn ExprPlanner>] {&[]}

Get expr planners when creating new planner #11485

Get expr planners when creating new planner #11485

Conversation

jayzhan211 commented Jul 16, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211 commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jul 17, 2024