minor fix: clean data type for negative operation #3370
Conversation
| arg: Arc<dyn PhysicalExpr>, | ||
| input_schema: &Schema, | ||
| ) -> Result<Arc<dyn PhysicalExpr>> { | ||
| // TODO: the data type checker should be done in the logical plan phase |
There was a problem hiding this comment.
If datafusion dose this in the preview stage or the logical plan generation stage, I think we don't need to check the child/input data type for the physical expr generation.
cc @alamb
Is there user case that user generate the physical plan or physical expr directly without the logical plan or logical stage?
There was a problem hiding this comment.
I am not sure if anyone creates physical Exprs directly without a logical_plan Expr.
However, I agree with this approach to throw an error if the physical types don't line up -- automatic coercion should be done at the logical level
There was a problem hiding this comment.
I am not sure if anyone creates physical Exprs directly without a logical_plan Expr.
However, I agree with this approach to throw an error if the physical types don't line up -- automatic coercion should be done at the logical level
Got it.
If the phy expr can be created directly without the type coercion or type checker in the logical phase, we need this checker in the creation of physical expr.
There was a problem hiding this comment.
If the phy expr can be created directly without the type coercion or type checker in the logical phase, we need this checker in the creation of physical expr.
That is a good point.
Maybe the larger question is if we are ok requiring that the logical optimizer passes are run on LogicalPlans and Expr to successfully create ExecutionPlans and PhysicalExprs,
Now that I type that it seems there is value in supporting creating Exprs and then PhysicalExprs directly (as part of implementing extensions, for example) 🤔 So maybe we should just ensure it is possible to call the coercion logic directly on a Expr
| Ok(ScalarValue::Decimal128(Some(-v), *precision, *scale)) | ||
| } | ||
| _ => panic!("Cannot run arithmetic negate on scalar value: {:?}", self), | ||
| value => Err(DataFusionError::Internal(format!( |
There was a problem hiding this comment.
replace the panic with the error
cc @andygrove
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
LGTM -- thanks @liukun4515
| arg: Arc<dyn PhysicalExpr>, | ||
| input_schema: &Schema, | ||
| ) -> Result<Arc<dyn PhysicalExpr>> { | ||
| // TODO: the data type checker should be done in the logical plan phase |
There was a problem hiding this comment.
I am not sure if anyone creates physical Exprs directly without a logical_plan Expr.
However, I agree with this approach to throw an error if the physical types don't line up -- automatic coercion should be done at the logical level
|
Benchmark runs are scheduled for baseline = 01d8730 and contender = 08a85db. 08a85db is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
minor fix for negative operation in
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?