alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1856449804
##########
datafusion-examples/examples/expr_api.rs:
##########
@@ -356,8 +357,13 @@ fn type_coercion_demo() -> Result<()> {
// Evaluation with an expression that has not been type coerced cannot
succeed.
let props = ExecutionProps::default();
- let physical_expr =
- datafusion_physical_expr::create_physical_expr(&expr, &df_schema,
&props)?;
+ let config_options = Arc::new(ConfigOptions::default());
+ let physical_expr = datafusion_physical_expr::create_physical_expr(
Review Comment:
It seems somewhat inevitable that creating a physical expr will require the
config options
However, I also think threading through the config options down through to
the physical creation will (finally) permit people to pass things from the
session down to function implementations (I think @cisaacson also was trying
to do this in the past)
##########
datafusion/core/src/datasource/listing/helpers.rs:
##########
@@ -283,10 +284,16 @@ async fn prune_partitions(
// TODO: Plumb this down
Review Comment:
This todo may have now be complete
##########
datafusion/expr/src/udf.rs:
##########
@@ -336,6 +337,8 @@ pub struct ScalarFunctionArgs<'a> {
// The return type of the scalar function returned (from `return_type` or
`return_type_from_exprs`)
// when creating the physical expression from the logical expression
pub return_type: &'a DataType,
+ // The config options which can be used to lookup configuration properties
+ pub config_options: Arc<ConfigOptions>,
Review Comment:
🎉
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -631,11 +631,18 @@ impl<'a> ConstEvaluator<'a> {
return ConstSimplifyResult::NotSimplified(s);
}
- let phys_expr =
- match create_physical_expr(&expr, &self.input_schema,
self.execution_props) {
- Ok(e) => e,
- Err(err) => return
ConstSimplifyResult::SimplifyRuntimeError(err, expr),
- };
+ // todo - should the config options be the actual options here or is
this sufficient?
Review Comment:
I think the actual configuration options are needed here. Otherwise what
will happen is that any function whose behavior relies on the ConfigOptions may
have different behavior on columns and constants (or other expressions that can
be constant folded)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
