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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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

datafusion/sql/src/planner.rs

    }

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

    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

    }

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

    }

+    /// 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 <jayzhan211@gmail.com>

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

datafusion/core/src/execution/session_state.rs


 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 <jayzhan211@gmail.com>

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

datafusion/core/src/execution/session_state.rs


 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 <jayzhan211@gmail.com> * get expr planner when creating planner Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * no planners in sqltorel Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * Add docs about SessionContextProvider * Use Slice rather than Vec to access expr planners * add test Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * clippy Signed-off-by: jayzhan211 <jayzhan211@gmail.com> --------- Signed-off-by: jayzhan211 <jayzhan211@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

get expr planners when creating new planner

ed18a5b

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

jayzhan211 commented Jul 17, 2024

View reviewed changes

clippy

9b34d21

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

alamb approved these changes Jul 17, 2024

View reviewed changes

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

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

	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

Uh oh!

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?

Uh oh!

jayzhan211 commented Jul 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjones127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants