alamb commented on a change in pull request #1700:
URL: https://github.com/apache/arrow-datafusion/pull/1700#discussion_r794710883
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1148,8 +1180,6 @@ pub struct ExecutionContextState {
pub catalog_list: Arc<dyn CatalogList>,
/// Scalar functions that are registered with the context
pub scalar_functions: HashMap<String, Arc<ScalarUDF>>,
- /// Variable provider that are registered with the context
- pub var_provider: HashMap<VarType, Arc<dyn VarProvider + Send + Sync>>,
Review comment:
while technically this is a breaking change, the
`ExecutionContext::register_variable` remains the same which I think means this
change will be minimally disruptive to people using DataFusion
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -238,13 +238,12 @@ pub struct ConstEvaluator {
/// descendants) so this Expr can be evaluated
can_evaluate: Vec<bool>,
- ctx_state: ExecutionContextState,
Review comment:
Here is reason 1 for this change: We no longer have to create a whole
new `ctx_state` and `planner` for each potential constant evaluation 🎉
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -299,12 +299,11 @@ impl PhysicalPlanner for DefaultPhysicalPlanner {
input_schema: &Schema,
ctx_state: &ExecutionContextState,
) -> Result<Arc<dyn PhysicalExpr>> {
- DefaultPhysicalPlanner::create_physical_expr(
- self,
+ create_physical_expr(
Review comment:
The actual interface for `PhysicalPlanner` remains the same; However it
is now also possible to call the free function `create_physical_expr` directly
too
##########
File path: datafusion/src/physical_optimizer/pruning.rs
##########
@@ -129,12 +130,14 @@ impl PruningPredicate {
.collect::<Vec<_>>();
let stat_schema = Schema::new(stat_fields);
let stat_dfschema = DFSchema::try_from(stat_schema.clone())?;
- let execution_context_state = ExecutionContextState::new();
- let predicate_expr =
DefaultPhysicalPlanner::default().create_physical_expr(
+
+ // TODO allow these properties to be passed in
Review comment:
Here is reason 2 for this change: again we save a whole new `ctx_state`
and `planner` for each potential constant evaluation 🎉
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -866,517 +865,487 @@ impl DefaultPhysicalPlanner {
exec_plan
}.boxed()
}
+}
- /// Create a physical expression from a logical expression
- pub fn create_physical_expr(
- &self,
- e: &Expr,
- input_dfschema: &DFSchema,
- input_schema: &Schema,
- ctx_state: &ExecutionContextState,
- ) -> Result<Arc<dyn PhysicalExpr>> {
- match e {
- Expr::Alias(expr, ..) => Ok(self.create_physical_expr(
- expr,
- input_dfschema,
- input_schema,
- ctx_state,
- )?),
- Expr::Column(c) => {
- let idx = input_dfschema.index_of_column(c)?;
- Ok(Arc::new(Column::new(&c.name, idx)))
- }
- Expr::Literal(value) => Ok(Arc::new(Literal::new(value.clone()))),
- Expr::ScalarVariable(variable_names) => {
- if &variable_names[0][0..2] == "@@" {
- match ctx_state.var_provider.get(&VarType::System) {
- Some(provider) => {
- let scalar_value =
- provider.get_value(variable_names.clone())?;
- Ok(Arc::new(Literal::new(scalar_value)))
- }
- _ => Err(DataFusionError::Plan(
- "No system variable provider found".to_string(),
- )),
+/// Create a physical expression from a logical expression ([Expr])
Review comment:
again, whitespace blind diff is the best to see -- this change makes
this a free function and changes the parameters from
```rust
ctx_state: &ExecutionContextState,
```
to
```rust
execution_props: &ExecutionProps,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
