Omega359 commented on code in PR #13356:
URL: https://github.com/apache/datafusion/pull/13356#discussion_r1839181975
##########
datafusion/sqllogictest/test_files/nullif.slt:
##########
@@ -97,11 +97,35 @@ SELECT NULLIF(1, 3);
----
1
-query I
+query T
SELECT NULLIF(NULL, NULL);
----
NULL
+query R
+select nullif(1, 1.2);
+----
+1
+
+query R
+select nullif(1.0, 2);
+----
+1
+
+query error DataFusion error: Error during planning: Internal error: Failed to
match any signature, errors: Error during planning: The signature expected
NativeType::String but received NativeType::Int64
Review Comment:
This used to work, I just ran this locally against v43. I can't see a reason
why this should no longer be supported.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -477,6 +483,33 @@ fn get_valid_types(
vec![vec![base_type_or_default_type(&coerced_type); *number]]
}
+ TypeSignature::Boolean(number) => {
+ function_length_check(current_types.len(), *number)?;
+
+ // Find common boolean type amongs given types
+ let mut valid_type = current_types.first().unwrap().to_owned();
Review Comment:
Unless I'm mistaken there is only one possible valid type here - Boolean
doesn't have multiple types, does it? If so, I don't see the need for this
variable nor the code below the for loop. valid_type must be DataType::Boolean,
no?
##########
datafusion/sqllogictest/test_files/nullif.slt:
##########
@@ -97,11 +97,35 @@ SELECT NULLIF(1, 3);
----
1
-query I
+query T
SELECT NULLIF(NULL, NULL);
----
NULL
+query R
+select nullif(1, 1.2);
+----
+1
+
+query R
+select nullif(1.0, 2);
+----
+1
+
+query error DataFusion error: Error during planning: Internal error: Failed to
match any signature, errors: Error during planning: The signature expected
NativeType::String but received NativeType::Int64
+select nullif(2, 'a');
+
+
+query T
+select nullif('2', '3');
+----
+2
+
+# TODO: support numeric string
+# This query success in Postgres and DuckDB
+query error DataFusion error: Error during planning: Internal error: Failed to
match any signature, errors: Error during planning: The signature expected
NativeType::String but received NativeType::Int64
+select nullif(2, '1');
Review Comment:
This also used to work.
query T
select nullif(2, '1');
----
2
Interesting that the type is text vs number but still, it did work.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -477,6 +483,33 @@ fn get_valid_types(
vec![vec![base_type_or_default_type(&coerced_type); *number]]
}
+ TypeSignature::Boolean(number) => {
+ function_length_check(current_types.len(), *number)?;
+
+ // Find common boolean type amongs given types
Review Comment:
```suggestion
// Find common boolean type amongst the given types
```
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -194,13 +195,18 @@ fn try_coerce_types(
// Well-supported signature that returns exact valid types.
if !valid_types.is_empty() && is_well_supported_signature(type_signature) {
- // exact valid types
- assert_eq!(valid_types.len(), 1);
+ // There may be many valid types if valid signature is OneOf
+ // Otherwise, there should be only one valid type
+ if !type_signature.is_one_of() {
+ assert_eq!(valid_types.len(), 1);
+ }
+
let valid_types = valid_types.swap_remove(0);
if let Some(t) = maybe_data_types_without_coercion(&valid_types,
current_types) {
return Ok(t);
}
} else {
+ // TODO: Deprecate this branch after all signatures are well-supported
(aka coercion are happend already)
Review Comment:
```suggestion
// TODO: Deprecate this branch after all signatures are
well-supported (aka coercion has happened already)
```
##########
datafusion/functions/src/core/nullif.rs:
##########
@@ -61,9 +41,13 @@ impl Default for NullIfFunc {
impl NullIfFunc {
pub fn new() -> Self {
Self {
- signature: Signature::uniform(
- 2,
- SUPPORTED_NULLIF_TYPES.to_vec(),
+ signature: Signature::one_of(
+ // Hack: String is at the beginning so the return type is
String if both args are Nulls
Review Comment:
Not sure I'd call that a tbhhack
--
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
