mingmwang commented on issue #6261: URL: https://github.com/apache/arrow-datafusion/issues/6261#issuecomment-1542354072
> agree with @jackwener > > I think type of expr should not be changed in the physical phase and all of type coercion should be done in the logical phase, but in current codebase, we can't do this. > > The current implementation of the logical expr is `enum` and each type of logical lacks the result type , which is not friendly to achieve above thought. > > In other SQL system, the logical expr is import part, each expr should has its input type and output type, and the output type need to be calculated from input types with operation. Once the common type and result are calculated in the logical phase, any changes for the type of expr should be forbidden. > > As the comments pointed by @jackwener, some expr can't be evaluated many times. For example, > > `old_expr = Add(left : decimal(10,2),right: decimal(10,2))` after evaluating the first time: the common type is decimal(11,2) left casted from decimal(10,2) to decimal(11,2) right casted from decimal(10,2) to decimal(11,2) the result type is decimal(11,2) > > and will get the `new_expr = Add(left: cast(decimal(10,2) to decimal(11,2)), right:cast(decimal(10,2) to decimal(11,2))` If we evaluate the `new_expr` again, the new result will be generated. > > @alamb > agree with @jackwener > > I think type of expr should not be changed in the physical phase and all of type coercion should be done in the logical phase, but in current codebase, we can't do this. > > The current implementation of the logical expr is `enum` and each type of logical lacks the result type , which is not friendly to achieve above thought. > > In other SQL system, the logical expr is import part, each expr should has its input type and output type, and the output type need to be calculated from input types with operation. Once the common type and result are calculated in the logical phase, any changes for the type of expr should be forbidden. > > As the comments pointed by @jackwener, some expr can't be evaluated many times. For example, > > `old_expr = Add(left : decimal(10,2),right: decimal(10,2))` after evaluating the first time: the common type is decimal(11,2) left casted from decimal(10,2) to decimal(11,2) right casted from decimal(10,2) to decimal(11,2) the result type is decimal(11,2) > > and will get the `new_expr = Add(left: cast(decimal(10,2) to decimal(11,2)), right:cast(decimal(10,2) to decimal(11,2))` If we evaluate the `new_expr` again, the new result will be generated. > > @alamb @liukun4515 @jackwener In the current latest implementation, this logic is moved out from the logical plan to physical plan. That means evaluating multiple times will generate idempotent result. I like the current implementation made by @viirya. -- 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]
