[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3662: change pre_cast_lit_in_comparison to unwrap_cast_in_comparison

[GitBox]

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: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org