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: [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]
