kosiew commented on code in PR #17418:
URL: https://github.com/apache/datafusion/pull/17418#discussion_r2329411507
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -316,6 +321,17 @@ impl<'a> BinaryTypeCoercer<'a> {
}
}
+#[inline]
+fn is_both_null(lhs: &DataType, rhs: &DataType) -> bool {
+ matches!(lhs, DataType::Null) && matches!(rhs, DataType::Null)
+}
+
+#[inline]
+fn is_arithmetic(op: &Operator) -> bool {
Review Comment:
The compiler already does some automatic inlining.
`#[inline]`
is usually used for cross-crate fn
and after demonstration of clear performance benefits.
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -124,17 +124,22 @@ impl<'a> BinaryTypeCoercer<'a> {
/// Returns a [`Signature`] for applying `op` to arguments of type `lhs`
and `rhs`
fn signature(&'a self) -> Result<Signature> {
+ // Special handling for arithmetic operations with both `lhs` and
`rhs` NULL:
+ // When both operands are NULL, we are providing a concrete numeric
type (Int64)
+ // to allow the arithmetic operation to proceed. This ensures NULL
`op` NULL returns NULL
+ // instead of failing during planning.
+ if is_both_null(self.lhs, self.rhs) && is_arithmetic(self.op) {
+ return Ok(Signature::uniform(DataType::Int64));
+ }
Review Comment:
You could inline `is_both_null` easier with this
```
- if is_both_null(self.lhs, self.rhs) && is_arithmetic(self.op) {
+ if matches!((self.lhs, self.rhs), (DataType::Null, DataType::Null))
+ && is_arithmetic(self.op)
+ {
```
##########
datafusion/sqllogictest/test_files/select.slt:
##########
@@ -620,6 +620,12 @@ select * from (values (1)) LIMIT 10*100;
----
1
+# select both nulls with basic arithmetic(modulo)
+query I
+select null % null;
+----
+NULL
+
Review Comment:
Consider moving this to operator.slt where arithmetic operator tests are
now.
The fix targets all arithmetic operators but only this only tests for
modulo.
Other operators remain untested for the NULL op NULL case
Consider adding coverage for other NULL arithmetic cases in operator.slt.
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -316,6 +321,17 @@ impl<'a> BinaryTypeCoercer<'a> {
}
}
+#[inline]
+fn is_both_null(lhs: &DataType, rhs: &DataType) -> bool {
+ matches!(lhs, DataType::Null) && matches!(rhs, DataType::Null)
+}
Review Comment:
See comment above.
--
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...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
