You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/24 13:53:37 UTC
[GitHub] [arrow-datafusion] liukun4515 opened a new issue #1356: The framework about expression type coercion
liukun4515 opened a new issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356
**Describe the bug**
In the [Implement DECIMAL type](https://github.com/apache/arrow-datafusion/issues/122), I want to implement the decimal data type in datafusion.
In the survey period, I take a look the type coercion in the current datafusion, and find some confused points.
In this issue, I want to discuss them.
For example:
- `AggFunction`
```
logical expr：
agg(input_exprs)
```
In the expression evaluation, we need to coercion the expr and calc the result type of `ScalarFunction` expr.
In the current code, the logical result type use this API[return_type](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/aggregates.rs#L97), but get the physical expr result type use this API[create_agg_expr](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/aggregates.rs#L121).
These two API have a gap, which is `coerce`. In the `coerce`, the input expr may be added `try_cast` and change the data type, so the inferred result data type may be diff with the logical result data type.
**To Reproduce**
Steps to reproduce the behavior:
**Expected behavior**
A clear and concise description of what you expected to happen.
**Additional context**
Add any other context about the problem here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978309526
I agree that the `coercion` framework is a bit messy at the moment. It might help to start refactoring by collecting the coercion logic into a single module with some clearly defined entrypoint.
Perhaps something like
```rust
pub fn coerce_to(expr: Expr, to_type: &DataType) -> Option<Expr> {
...
}
```
With appropriate comments and tests
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978856798
In the binary expr, the input has the `op type` and `left,right expr`, we need these three component to get the `result type` and `coercion`;
In the build-in function expr, the input has the `function type` and `exprs`, we need these three component to get the `result type` and `coercion`.
The `AggregateFunction` is the same as above.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-979917158
> Now, the coercision is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb
Yes, this is something @jorgecarleitao wrote -- I think it originally came in as part of https://github.com/apache/arrow/pull/8024, which has much more of the rationale. Other systems I have worked on do the coercion at the `LogicalPlan` / `Expr` level, but I think doing coercion at the `ExeuctioPlan` / `PhysicalExpr` level makes sense too. Unless we discover some compelling reason to move coercion to `LogicalPlan` I suggest we do not revisit this decision.
> I think we can create a new mod for coercion and type inferring.
> The new mod should be pub crate and should not public to out of the datafusion crate.
This makes sense to me
> I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.
Yes I agree
> In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.
I think it might make sense to keep all the coercion logic in the same place (e.g. in the `coercion.rs` module rather than splitting it across `AggFunction`, `ScalarFunction`, etc) but I do not feel super strongly about this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-1021642866
I think this issue is now closed and we are on our way to uniform type coercion logic ❤️
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb closed issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
alamb closed issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978669578
Now, the `coercision` is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-977915679
By the way, I think there is a [bug](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/aggregates.rs#L138).
```
let name = name.into();
let arg = coerce(args, input_schema, &signature(fun))?;
if arg.is_empty() {
return Err(DataFusionError::Plan(format!(
"Invalid or wrong number of arguments passed to aggregate: '{}'",
name,
)));
}
let arg = arg[0].clone();
let arg_types = args
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
let return_type = return_type(fun, &arg_types)?;
```
The `args` is the original physical exprs, and the `arg` is the coerced physical exprs.
In order to get the result type, we need replace
```
let arg_types = args
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
```
with
```
let arg_types = arg
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
```
In the [functions.rs](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/functions.rs#L1139), I think the usage is right.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978315168
BTW I would love to help centralize the coercion logic -- I have ideas / plans to make it support coercing to/from NULL which is currently a gap.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-1021780614
> I think this issue is now closed and we are on our way to uniform type coercion logic ❤️
thank you for your reminder.
I will focus on the coercion logic when we meet some issues.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-979972001
> > Now, the coercision is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb
>
> Yes, this is something @jorgecarleitao wrote -- I think it originally came in as part of [apache/arrow#8024](https://github.com/apache/arrow/pull/8024), which has much more of the rationale. Other systems I have worked on do the coercion at the `LogicalPlan` / `Expr` level, but I think doing coercion at the `ExeuctioPlan` / `PhysicalExpr` level makes sense too. Unless we discover some compelling reason to move coercion to `LogicalPlan` I suggest we do not revisit this decision.
>
Yes, now we have not meet some issues which should change the coercion to logical period. if we meet them, we can discuss this again.
> > I think we can create a new mod for coercion and type inferring.
> > The new mod should be pub crate and should not public to out of the datafusion crate.
>
> This makes sense to me
>
> > I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.
>
> Yes I agree
>
> > In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.
>
> I think it might make sense to keep all the coercion logic in the same place (e.g. in the `coercion.rs` module rather than splitting it across `AggFunction`, `ScalarFunction`, etc) but I do not feel super strongly about this
I think we have aligned that we can split coercion logic into a independent mod (one sub directory in the `physical_plan` directory).
I will try to implement the Agg function to as a sample.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 edited a comment on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 edited a comment on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-977915679
By the way, I think there is a [bug](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/aggregates.rs#L138) and #1358
```
let name = name.into();
let arg = coerce(args, input_schema, &signature(fun))?;
if arg.is_empty() {
return Err(DataFusionError::Plan(format!(
"Invalid or wrong number of arguments passed to aggregate: '{}'",
name,
)));
}
let arg = arg[0].clone();
let arg_types = args
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
let return_type = return_type(fun, &arg_types)?;
```
The `args` is the original physical exprs, and the `arg` is the coerced physical exprs.
In order to get the result type, we need replace
```
let arg_types = args
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
```
with
```
let arg_types = arg
.iter()
.map(|e| e.data_type(input_schema))
.collect::<Result<Vec<_>>>()?;
```
In the [functions.rs](https://github.com/apache/arrow-datafusion/blob/645a7f484435d7182d88a541ac8cdf8b079b7714/datafusion/src/physical_plan/functions.rs#L1139), I think the usage is right.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-979654961
@jorgecarleitao Do you have time to take look it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978685756
> BTW I would love to help centralize the coercion logic -- I have ideas / plans to make it support coercing to/from NULL which is currently a gap.
Can you explain or give more details?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978791177
> I agree that the `coercion` framework is a bit messy at the moment. It might help to start refactoring by collecting the coercion logic into a single module with some clearly defined entrypoint.
>
> Perhaps something like
>
> ```rust
> pub fn coerce_to(expr: Expr, to_type: &DataType) -> Option<Expr> {
> ...
> }
> ```
>
> With appropriate comments and tests
Now all the coercion logic are in one the rule file, and it can't fit many case. For example
```
select abs('123');
select '123'+10;
select 12.3+'1.23';
....
```
Now all the coercion logic are in one the rule file, and it can't fit many case. For example
I think we can create a new mod for coercion and type inferring.
The new mod should be `pub crate` and should not public to out of the `datafusion` crate.
I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.
In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on issue #1356: The framework about expression type coercion
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #1356:
URL: https://github.com/apache/arrow-datafusion/issues/1356#issuecomment-978858270
@andygrove @Jimexist @Dandandan @houqp PTAL
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org