alamb commented on code in PR #3662:
URL: https://github.com/apache/arrow-datafusion/pull/3662#discussion_r984479957
##########
datafusion/core/tests/sql/explain_analyze.rs:
##########
@@ -779,23 +777,6 @@ async fn csv_explain() {
// Note can't use `assert_batches_eq` as the plan needs to be
// normalized for filenames and number of cores
- let expected = vec![
- vec![
- "logical_plan",
- "Projection: #aggregate_test_100.c1\
- \n Filter: CAST(#aggregate_test_100.c2 AS Int32) > Int32(10)\
- \n TableScan: aggregate_test_100 projection=[c1, c2],
partial_filters=[CAST(#aggregate_test_100.c2 AS Int32) > Int32(10)]"
Review Comment:
The key difference for anyone else following along is that
```
partial_filters=[CAST(#aggregate_test_100.c2 AS Int32) > Int32(10)]
```
Has become
```
partial_filters=[partial_filters=[#aggregate_test_100.c2 > Int8(10)]
```
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1466,9 +1466,9 @@ impl SessionState {
}
let mut rules: Vec<Arc<dyn OptimizerRule + Sync + Send>> = vec![
- Arc::new(PreCastLitInComparisonExpressions::new()),
Arc::new(TypeCoercion::new()),
Arc::new(SimplifyExpressions::new()),
+ Arc::new(UnwrapCastInComparison::new()),
Review Comment:
this name is much easier to understand for me -- thank you
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -540,10 +602,10 @@ mod tests {
);
assert_eq!(optimize_test(expr_lt, &schema), expected);
- // INT32(12) IN (.....)
- let expr_lt = lit(ScalarValue::Int32(Some(12))).in_list(
+ // cast(INT32(12), INT64) IN (.....)
+ let expr_lt = cast(lit(ScalarValue::Int32(Some(12))),
DataType::Int64).in_list(
vec![
- lit(ScalarValue::Int32(Some(12))),
+ lit(ScalarValue::Int64(Some(12))),
Review Comment:
Is this change (so that all the `IN` list types are the same) because type
coercion will have already been done when this code is now called?
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -15,8 +15,9 @@
// specific language governing permissions and limitations
// under the License.
-//! Pre-cast literal binary comparison rule can be only used to the binary
comparison expr.
-//! It can reduce adding the `Expr::Cast` to the expr instead of adding the
`Expr::Cast` to literal expr.
+//! Unwrap-cast binary comparison rule can be used to the binary/inlist
comparison expr now, and other type
+//! of expr can be added if needed.
+//! This rule can reduce adding the `Expr::Cast` the expr instead of adding
the `Expr::Cast` to literal expr.
Review Comment:
```suggestion
//! Unwrap-cast binary comparison rule can be used to remove `cast`
functions
//! from column references.
//!
//! The reason for doing so is that it is often much more efficient to
//! evaluate a predicate like `c1 < literal` via specialized kernels or
pruning
//! rather than `cast(c1 as TYPE) < literal`
```
--
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]
