You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "skushagra (via GitHub)" <gi...@apache.org> on 2025/12/07 17:50:06 UTC
[PR] fix: custom nullability for length (#19175) [datafusion]
skushagra opened a new pull request, #19182:
URL: https://github.com/apache/datafusion/pull/19182
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #19175
## What changes are included in this PR?
- includes custom nullability for length, determined dynamically using input arguments.
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596485794
##########
datafusion/spark/src/function/string/length.rs:
##########
@@ -92,6 +93,11 @@ impl ScalarUDFImpl for SparkLengthFunc {
fn aliases(&self) -> &[String] {
&self.aliases
}
+
+ fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> {
+ let nullable = args.arg_fields.iter().any(|f| f.is_nullable());
+ Ok(Arc::new(Field::new(self.name(), Boolean, nullable)))
Review Comment:
this function does not return a boolean but Int32, see the `return_type` function above.
can you please also add a test that it is nullable when the child is nullable
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#issuecomment-3622705141
I've verified that this expression is null intolerant:
https://github.com/apache/spark/blob/98058da21e8a341eca10207f0ca458671220ca94/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L2447
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596537530
##########
datafusion/spark/src/function/string/length.rs:
##########
@@ -279,4 +286,38 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_spark_length_nullability() -> Result<()> {
+ let func = SparkLengthFunc::new();
+
+ let nullable_field: FieldRef =
+ Arc::new(Field::new("col", DataType::Utf8, true));
+
+ let out_nullable = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[nullable_field],
+ scalar_arguments: &[],
+ })?;
+
+ assert!(
+ out_nullable.is_nullable(),
+ "length(col) should be nullable when child is nullable"
+ );
+
+ let non_nullable_field: FieldRef =
+ Arc::new(Field::new("col", DataType::Utf8, false));
+
+ let out_non_nullable = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[non_nullable_field],
+ scalar_arguments: &[],
Review Comment:
```suggestion
scalar_arguments: &[None],
```
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton merged PR #19182:
URL: https://github.com/apache/datafusion/pull/19182
--
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: custom nullability for length (#19175) [datafusion]
Posted by "skushagra (via GitHub)" <gi...@apache.org>.
skushagra commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596498279
##########
datafusion/spark/src/function/string/length.rs:
##########
Review Comment:
what should the error message say exactly?
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596486238
##########
datafusion/spark/src/function/string/length.rs:
##########
Review Comment:
please move the comment to `return_field_from_args`, and return `internal_err!` that this should not be called, and `return_field_from_args` should be called instead
--
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: custom nullability for length (#19175) [datafusion]
Posted by "skushagra (via GitHub)" <gi...@apache.org>.
skushagra commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596498279
##########
datafusion/spark/src/function/string/length.rs:
##########
Review Comment:
what should the error message say exactly? Going with
`internal_err!("This should not be called, use return_field_from_args instead")` for now.
--
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: custom nullability for length (#19175) [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #19182:
URL: https://github.com/apache/datafusion/pull/19182#discussion_r2596537420
##########
datafusion/spark/src/function/string/length.rs:
##########
@@ -279,4 +286,38 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_spark_length_nullability() -> Result<()> {
+ let func = SparkLengthFunc::new();
+
+ let nullable_field: FieldRef =
+ Arc::new(Field::new("col", DataType::Utf8, true));
+
+ let out_nullable = func.return_field_from_args(ReturnFieldArgs {
+ arg_fields: &[nullable_field],
+ scalar_arguments: &[],
Review Comment:
there should be the same number of `scalar_arguments` and `arg_fields`, if something is not scalar than None is used.
```suggestion
scalar_arguments: &[None],
```
--
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