You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ShashidharM0118 (via GitHub)" <gi...@apache.org> on 2025/12/15 06:50:57 UTC
[PR] fix: derive Spark sha2 nullability and add tests [datafusion]
ShashidharM0118 opened a new pull request, #19323:
URL: https://github.com/apache/datafusion/pull/19323
# Which issue does this PR close?
Closes #19159
# Rationale for this change
Align Spark `sha2` nullability with Spark semantics; output reflects nullable inputs/scalars.
# What changes are included in this PR?
- Derive return field via `return_field_from_args`, using input types/nullability (including null scalars).
- Stub `return_type` to point to `return_field_from_args`.
- Update imports for `Field`, `FieldRef`, `ReturnFieldArgs`, `internal_err`.
- Add nullability unit test `test_sha2_nullability`.
# Are these changes tested?
Yes,
# Are there any user-facing changes?
No
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on code in PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#discussion_r2622732219
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
Review Comment:
> Please add comments above each case. I don't see what is the difference between nullable_expr (https://github.com/apache/datafusion/pull/19323/changes#diff-288c6716efda738c6ce1f71da2e679e600cfa79b78aeadd7871ce063d701d334R292-R302) and null_expr (https://github.com/apache/datafusion/pull/19323/changes#diff-288c6716efda738c6ce1f71da2e679e600cfa79b78aeadd7871ce063d701d334R316-R322)
Hi @martin-g, thanks for the review.
The goal was to cover both branches of this part of `return_field_from_args`:
```
match arg_types[0] {
DataType::Utf8View
| DataType::LargeUtf8
| DataType::Utf8
| DataType::Binary
| DataType::BinaryView
| DataType::LargeBinary => DataType::Utf8,
DataType::Null => DataType::Null,
}
```
For the `nullable_expr` case I picked one representative type from the “string / binary” group above, `DataType::Binary`:
- `expr = Field::new("expr", DataType::Binary, /* nullable = */ true)`
- `bit_length = Int32 (non-null)`
- This hits the first arm and checks we return **`Utf8` and nullable** (nullable Binary → nullable Utf8).
For the `null_expr` case I picked the only type from the second arm:
- `expr = Field::new("expr", DataType::Null, /* nullable = */ true)`
- `bit_length = Int32 (non-null)`
- This hits the `DataType::Null => DataType::Null` arm and checks we return **`Null` and nullable** (Null expr type → Null result type).
So the two tests are intentionally chosen representatives: one from the “string/binary → Utf8” branch and one from the “Null → Null” branch, to make sure both type paths are covered.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#discussion_r2618652611
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -65,25 +69,44 @@ impl ScalarUDFImpl for SparkSha2 {
&self.signature
}
- fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if arg_types[1].is_null() {
- return Ok(DataType::Null);
- }
- Ok(match arg_types[0] {
- DataType::Utf8View
- | DataType::LargeUtf8
- | DataType::Utf8
- | DataType::Binary
- | DataType::BinaryView
- | DataType::LargeBinary => DataType::Utf8,
- DataType::Null => DataType::Null,
- _ => {
- return exec_err!(
- "{} function can only accept strings or binary arrays.",
- self.name()
- );
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ internal_err!("return_field_from_args should be used instead")
+ }
+
+ fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> {
+ let arg_types: Vec<_> = args
+ .arg_fields
+ .iter()
+ .map(|field| field.data_type().clone())
+ .collect();
+
+ let data_type = if arg_types[1].is_null() {
+ DataType::Null
+ } else {
+ match arg_types[0] {
+ DataType::Utf8View
+ | DataType::LargeUtf8
+ | DataType::Utf8
+ | DataType::Binary
+ | DataType::BinaryView
+ | DataType::LargeBinary => DataType::Utf8,
+ DataType::Null => DataType::Null,
+ _ => {
+ return exec_err!(
+ "{} function can only accept strings or binary arrays.",
+ self.name()
+ );
+ }
}
- })
+ };
+
+ let nullable = args.arg_fields.iter().any(|f| f.is_nullable())
+ || args
+ .scalar_arguments
+ .iter()
+ .any(|scalar| scalar.is_some_and(|s| s.is_null()));
Review Comment:
I think this should also check that the second argument's (bit_length) value is one of the supported ones (0, 224, 256, 384, or 512), because for invalid values it returns `ScalarValue::Utf8(None)` at https://github.com/ShashidharM0118/datafusion/blob/54561641ae94d9100d267477511127745ac51f60/datafusion/spark/src/function/hash/sha2.rs#L246
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#issuecomment-3680563232
Thanks @ShashidharM0118 & @martin-g, but seems these changes aren't required
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey closed pull request #19323: fix: derive Spark sha2 nullability and add tests
URL: https://github.com/apache/datafusion/pull/19323
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#issuecomment-3679746916
@rluvaton are we sure on this requirement? I took a look at Spark code and it seems to be nullable, unless I'm reading it wrong/looking at wrong place:
https://github.com/apache/spark/blob/20af8bdfb9073be5711cea5df0c5ce0ff168e92c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala#L107-L114
Also we need to consider when the bit lengths is an array; if there is an invalid bit length it should null, see this example test:
```sql
statement ok
CREATE TABLE test_table (
expr STRING NOT NULL,
bit_length INT NOT NULL
) as VALUES
('foo', 0),
('foo', 999)
;
query T
select sha2(arrow_cast(expr, 'Utf8'), bit_length) from test_table;
----
2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae
NULL
```
So deriving nullable = false would only work if the bit_length is a constant scalar.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#issuecomment-3680483417
You are right, some expressions I was wrong
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#discussion_r2622132402
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_bit_length: FieldRef =
Review Comment:
What is the difference with https://github.com/apache/datafusion/pull/19323/changes#diff-288c6716efda738c6ce1f71da2e679e600cfa79b78aeadd7871ce063d701d334R304-R314 ?
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[Arc::clone(&non_nullable_expr), null_bit_length],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_scalar = ScalarValue::Int32(None);
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
Review Comment:
```suggestion
Arc::new(&non_nullable_bit_length),
```
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[Arc::clone(&non_nullable_expr), null_bit_length],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_scalar = ScalarValue::Int32(None);
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
+ ],
+ scalar_arguments: &[None, Some(&null_scalar)],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let unsupported_scalar = ScalarValue::Int32(Some(128));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::new(Field::new("expr", DataType::Binary, false)),
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
Review Comment:
```suggestion
Arc::new(&non_nullable_bit_length),
```
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
Review Comment:
Please add comments above each case.
I don't see what is the difference between nullable_expr (https://github.com/apache/datafusion/pull/19323/changes#diff-288c6716efda738c6ce1f71da2e679e600cfa79b78aeadd7871ce063d701d334R292-R302) and null_expr (https://github.com/apache/datafusion/pull/19323/changes#diff-288c6716efda738c6ce1f71da2e679e600cfa79b78aeadd7871ce063d701d334R316-R322)
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +264,94 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_expr: FieldRef = Arc::new(Field::new("expr", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[null_expr, Arc::clone(&non_nullable_bit_length)],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[Arc::clone(&non_nullable_expr), null_bit_length],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_scalar = ScalarValue::Int32(None);
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
+ ],
+ scalar_arguments: &[None, Some(&null_scalar)],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let unsupported_scalar = ScalarValue::Int32(Some(128));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::new(Field::new("expr", DataType::Binary, false)),
Review Comment:
```suggestion
Arc::new(&non_nullable_expr),
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: derive Spark sha2 nullability and add tests [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19323:
URL: https://github.com/apache/datafusion/pull/19323#discussion_r2618194393
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +248,75 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[non_nullable_expr, null_bit_length],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_scalar = ScalarValue::Int32(None);
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ nullable_expr,
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
+ ],
+ scalar_arguments: &[None, Some(&null_scalar)],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
Review Comment:
This test case uses `nullable_expr` which is already defined as nullable (line 276). This means the result would be nullable even without the null scalar in `scalar_arguments`. To properly test that a null scalar makes the result nullable, this test should use `non_nullable_expr` instead of `nullable_expr` (similar to line 292 or 303).
##########
datafusion/spark/src/function/hash/sha2.rs:
##########
@@ -225,3 +248,75 @@ fn compute_sha2(
}
.map(|hashed| spark_sha2_hex(&[hashed]).unwrap())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::datatypes::Field;
+
+ #[test]
+ fn test_sha2_nullability() -> Result<()> {
+ let func = SparkSha2::new();
+
+ let non_nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, false));
+ let non_nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, false));
+
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(!out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_expr: FieldRef =
+ Arc::new(Field::new("expr", DataType::Binary, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&nullable_expr),
+ Arc::clone(&non_nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let nullable_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Int32, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ Arc::clone(&non_nullable_expr),
+ Arc::clone(&nullable_bit_length),
+ ],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ let null_bit_length: FieldRef =
+ Arc::new(Field::new("bit_length", DataType::Null, true));
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[non_nullable_expr, null_bit_length],
+ scalar_arguments: &[None, None],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Null);
+
+ let null_scalar = ScalarValue::Int32(None);
+ let out = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[
+ nullable_expr,
+ Arc::new(Field::new("bit_length", DataType::Int32, false)),
+ ],
+ scalar_arguments: &[None, Some(&null_scalar)],
+ })?;
+ assert!(out.is_nullable());
+ assert_eq!(out.data_type(), &DataType::Utf8);
+
+ Ok(())
+ }
Review Comment:
The test coverage should include a case where the first argument (expr) has DataType::Null while the second argument (bit_length) is a regular non-null Int32. This would verify that when arg_types[0] is DataType::Null, the function correctly returns DataType::Null as per line 93 of the implementation.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org