neilconway commented on code in PR #20306:
URL: https://github.com/apache/datafusion/pull/20306#discussion_r2799336096
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -744,18 +744,47 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
});
Ok(Transformed::yes(new_expr))
}
+ // IS NULL / IS NOT NULL / SIMILAR TO / CAST / TRY_CAST validate
Review Comment:
I don't think comments should reference "previously" (previously to what?);
they should just talk about the current state of the code.
I'd rephrase this comment to something like:
```
// IS NULL, IS NOT NULL, SIMILAR TO, CAST, and TRY CAST don't require
// type coercion, but we need to handle them here to ensure that their
// arguments have type coercion and type checking applied (see #20201
// for context).
```
##########
datafusion/sqllogictest/test_files/type_coercion.slt:
##########
@@ -281,3 +281,30 @@ SELECT true FROM t0 WHERE (REGEXP_MATCH(t0.v1, t0.v1)) NOT
ILIKE [];
statement ok
DROP TABLE t0;
+
+###############################################################################
+## Test that IS NULL / IS NOT NULL / SIMILAR TO / CAST / TRY_CAST validate ##
+## their inner expressions during type coercion (previously they were in ##
Review Comment:
I'm omit discussion of the previous incorrect behavior, I'd just talk about
the current/correct behavior under test.
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -2742,4 +2771,69 @@ mod test {
"
)
}
+
+ fn zero_arg_scalar_func() -> Expr {
+ let fun = ScalarUDF::new_from_impl(TestScalarUDF {
+ signature: Signature::uniform(1, vec![DataType::Float32],
Volatility::Stable),
+ });
+ Expr::ScalarFunction(ScalarFunction::new_udf(Arc::new(fun), vec![]))
+ }
+
+ #[test]
Review Comment:
These tests feel redundant to the SLT tests, no? In my opinion we should be
fine with just adding the SLT tests only (which arguably is closer to testing
the user-visible behavior anyway).
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -744,18 +744,47 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
});
Ok(Transformed::yes(new_expr))
}
+ // IS NULL / IS NOT NULL / SIMILAR TO / CAST / TRY_CAST validate
+ // their inner expressions during type coercion (previously they
were in
+ // the catch-all branch and skipped validation, which caused
panics at
+ // runtime for invalid inner expressions like zero-argument
function calls).
+ // Detail: <https://github.com/apache/datafusion/issues/20201>
+ //
+ // TODO: reject the invalid cases at planning.
+ Expr::IsNotNull(ref inner_expr) | Expr::IsNull(ref inner_expr) => {
+ let _check_type = inner_expr.get_type(self.schema)?;
+ Ok(Transformed::no(expr))
+ }
+ Expr::SimilarTo(Like {
Review Comment:
I wonder whether the handling of `SIMILAR TO` should be more similar to how
we handle `LIKE`?
--
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]
