-
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
Introduce user defined SQL planner API #11180
Changes from 3 commits
a7c9e7f
3c6801c
3362985
42f6414
8ece394
d5d3189
cdf5342
d386702
24372e4
e889009
10477a8
b662b1e
160e032
c4e2b33
dafd53e
5e9af66
a0786eb
5963626
f1f1b4c
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 |
---|---|---|
|
@@ -109,7 +109,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |
let mut binary_expr = datafusion_expr::planner::RawBinaryExpr { op, left, right }; | ||
for planner in self.planners.iter() { | ||
match planner.plan_binary_op(binary_expr, schema)? { | ||
PlannerResult::Simplified(expr) => { | ||
PlannerResult::Planned(expr) => { | ||
return Ok(expr); | ||
} | ||
PlannerResult::Original(expr) => { | ||
|
@@ -281,14 +281,14 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |
let mut field_access_expr = RawFieldAccessExpr { expr, field_access }; | ||
for planner in self.planners.iter() { | ||
match planner.plan_field_access(field_access_expr, schema)? { | ||
PlannerResult::Simplified(expr) => return Ok(expr), | ||
PlannerResult::Planned(expr) => return Ok(expr), | ||
PlannerResult::Original(expr) => { | ||
field_access_expr = expr; | ||
} | ||
} | ||
} | ||
|
||
internal_err!("Expected a simplified result, but none was found") | ||
not_impl_err!("GetFieldAccess not supported by UserDefinedExtensionPlanners: {field_access_expr:?}") | ||
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'm curious about this default branch because I'm seeing this error when doing integration testing with the delta-rs package. To the best of my knowledge we're not using any extension planners, but now this is failing with the latest datafusion. I'm uncertain whether we're doing something wrong, or if the default behavior of falling through to 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. Hi @rtyler -- looks to me like the delta.rs code is managing its own SessionState -- like in https://github.com/delta-io/delta-rs/blob/main/crates/core/src/delta_datafusion/mod.rs#L1687 (BTW the Delta API is really nicely thought out) So I think you'll need to register the same planners in your SessionContext 🤔 Helpfully I think @Omega359 just made a PR to make this easier: #11296 I feel in general DataFusion is hard to use / configure correctly if you are using a custom SessionState / configuration -- one potential thing we were discussing is #11182 (comment) -- I'll file a ticket to make this a thing. 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. 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. Proposed fix is here: #11485 |
||
} | ||
|
||
SQLExpr::CompoundIdentifier(ids) => { | ||
|
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 think we don't need Clone 🤔 Same as
RawFieldAccessExpr
andPlannerResult
. Others looks good to meThere 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 agree there isn't a critical usecase for it now, but I figured it didn't hurt. If you feel strongly I will remove
Clone
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 think it is not a critical issue.