Copilot commented on code in PR #19434:
URL: https://github.com/apache/datafusion/pull/19434#discussion_r2637615241
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -1443,10 +1443,25 @@ fn build_predicate_expression(
// predicate expression can only be a binary expression
let expr_any = expr.as_any();
if let Some(is_null) = expr_any.downcast_ref::<phys_expr::IsNullExpr>() {
+ // If argument is a literal, evaluate directly
+ if let Some(literal) =
is_null.arg().as_any().downcast_ref::<phys_expr::Literal>()
+ {
+ let result = literal.value().is_null();
+ return
Arc::new(phys_expr::Literal::new(ScalarValue::Boolean(Some(result))));
+ }
return build_is_null_column_expr(is_null.arg(), schema,
required_columns, false)
.unwrap_or_else(|| unhandled_hook.handle(expr));
}
if let Some(is_not_null) =
expr_any.downcast_ref::<phys_expr::IsNotNullExpr>() {
+ // If argument is a literal, evaluate directly
+ if let Some(literal) = is_not_null
+ .arg()
+ .as_any()
+ .downcast_ref::<phys_expr::Literal>()
+ {
+ let result = !literal.value().is_null();
+ return
Arc::new(phys_expr::Literal::new(ScalarValue::Boolean(Some(result))));
+ }
Review Comment:
The logic for simplifying IS NULL and IS NOT NULL expressions with literal
arguments is duplicated between this location and the new
`simplify_is_null_expr` function in
`datafusion/physical-expr/src/simplifier/is_null.rs`. While this duplication
may provide a safety net for code paths that don't go through the
PhysicalExprSimplifier, consider whether both implementations are necessary. If
both are needed, consider adding a comment explaining why this local
simplification is required in addition to the simplifier pipeline.
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -1443,10 +1443,25 @@ fn build_predicate_expression(
// predicate expression can only be a binary expression
let expr_any = expr.as_any();
if let Some(is_null) = expr_any.downcast_ref::<phys_expr::IsNullExpr>() {
+ // If argument is a literal, evaluate directly
+ if let Some(literal) =
is_null.arg().as_any().downcast_ref::<phys_expr::Literal>()
+ {
+ let result = literal.value().is_null();
+ return
Arc::new(phys_expr::Literal::new(ScalarValue::Boolean(Some(result))));
+ }
return build_is_null_column_expr(is_null.arg(), schema,
required_columns, false)
.unwrap_or_else(|| unhandled_hook.handle(expr));
}
if let Some(is_not_null) =
expr_any.downcast_ref::<phys_expr::IsNotNullExpr>() {
+ // If argument is a literal, evaluate directly
+ if let Some(literal) = is_not_null
+ .arg()
+ .as_any()
+ .downcast_ref::<phys_expr::Literal>()
+ {
+ let result = !literal.value().is_null();
+ return
Arc::new(phys_expr::Literal::new(ScalarValue::Boolean(Some(result))));
+ }
Review Comment:
The new simplification logic for IS NULL and IS NOT NULL with literal
arguments added in build_predicate_expression is not covered by tests in the
pruning module. Consider adding test cases that specifically verify this
behavior, such as testing IS NULL(NULL) and IS NOT NULL(NULL) expressions that
would result from schema evolution scenarios.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
