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 2020/08/29 04:12:47 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
jorgecarleitao opened a new pull request #8076:
URL: https://github.com/apache/arrow/pull/8076
This is a small simplification of the code that I found while reviewing the code. There were two different problems being addressed at the same function, and there was some un-needed argument in one function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480400554
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
I don't have any opinion on how the context of the error gets into the error message, but it does seem important to me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480022925
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
Can you explain why you removed the mention of which operator (`op`) was affected in the error message?
There is probably some reason, but on the face of it, this PR seems to make the errors less specific as now there is no hint to a user what operator is having the problem - just "somewhere in your expressions I couldn't coerce types"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#issuecomment-683233585
https://issues.apache.org/jira/browse/ARROW-9885
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480611387
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
I agree that this seems like a regression in UX. It is useful to know which operator was involved to reduce the effort to debug an error like 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8076:
URL: https://github.com/apache/arrow/pull/8076
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r481049867
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -1740,6 +1710,35 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_coersion_error() -> Result<()> {
+ let expr =
+ common_binary_type(&DataType::Float32, &Operator::Plus, &DataType::Utf8);
+
+ if let Err(ExecutionError::General(e)) = expr {
+ assert_eq!(e, "'Float32 + Utf8' can't be evaluated because there isn't a common type to coerce the types to");
Review comment:
👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480820539
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
I also agree. I've added a test to the error message to contain the operator, and made code pass the test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480399982
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
I guess I was thinking about the error from the user's perspective rather than the implementation. If I were a user and I got this error message, I want to know what part of my query / expression to fix.
For example, if I had `(A + B) - (C * D)` and I got "can't coerce int to float" error I am not sure I would know if the problem was with `A + B`or with `C * D` or with the overall difference. While the ideal error message would perhaps include an offset and a partial output of the original string, but having the operator name would make it easier to track down what I needed to do to fix my query
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8076: ARROW-9885: [Rust][DataFusion] Minor code simplification
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8076:
URL: https://github.com/apache/arrow/pull/8076#discussion_r480293094
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -991,48 +991,36 @@ impl fmt::Display for BinaryExpr {
}
}
-// Returns a formatted error about being impossible to coerce types for the binary operator.
-fn coercion_error<T>(
- lhs_type: &DataType,
- op: &Operator,
- rhs_type: &DataType,
-) -> Result<T> {
+// Returns a formatted error about being impossible to coerce types to a common type
+fn coercion_error<T>(lhs_type: &DataType, rhs_type: &DataType) -> Result<T> {
Err(ExecutionError::General(
format!(
- "The binary operator '{}' can't evaluate with lhs = '{:?}' and rhs = '{:?}'",
Review comment:
The message was re-formulated because it made it a bit clearer that the coercion did not fail due to the use of the types on a specific operator: any operator would have yielded the same error, and thus I dropped the operator from the message altogether.
This was a side effect of removing the operator from the coercions' functuons. Essentially, code such as `numerical_coercion(lhs_type: &DataType, op: Operator, rhs_type: &DataType)` suggests that coercion depends on the operator, but this is not true: only the error message depends on the operator.
However, I do agree with you that that is a generic message. We could wrap the error and re-write it with the operator's information. E.g.
```
Operator::Plus | Operator::Minus | Operator::Divide | Operator::Multiply => {
numerical_coercion(lhs_type, rhs_type)
}
```
could be a place to catch the error and re-write the error message with the operator's information. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org