NGA-TRAN commented on code in PR #4490:
URL: https://github.com/apache/arrow-datafusion/pull/4490#discussion_r1042193338
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -23,12 +23,20 @@ use crate::{aggregate_function, function, window_function};
use arrow::compute::can_cast_types;
use arrow::datatypes::DataType;
use datafusion_common::{DFField, DFSchema, DataFusionError, ExprSchema,
Result};
+use log::debug;
/// trait to allow expr to typable with respect to a schema
pub trait ExprSchemable {
/// given a schema, return the type of the expr
fn get_type<S: ExprSchema>(&self, schema: &S) -> Result<DataType>;
+ /// given a schema and param data types, return the type of the expr
+ fn get_type_with_params<S: ExprSchema>(
Review Comment:
@alamb : Yes, at first I planned to add the parameter into `get_type`
function but then I saw a massive change and I thought this solution would be
preferable but I agree there is a trade-off that if we miss using it, it will
lead to bugs.
I can make a massive change easily if reviewers agree to do so and I think
it will solve all edge cases and won't lead to bugs because we pass it all the
way down from statement where we know whether the parameter is (non-empty or
empty vector of params of a PlannerContext)
Let me have a quick look to see if moving parameter into DFSchema is a good
option. I am thinking it will lead to the same dilemma in the code somewhere
--
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]
