alamb commented on code in PR #3608:
URL: https://github.com/apache/arrow-datafusion/pull/3608#discussion_r979394639
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -816,6 +815,23 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a,
S> {
out_expr.rewrite(self)?
}
+ // concat_ws
Review Comment:
```suggestion
// concat_ws
// If any of the arguments is NULL, the output will be null
```
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -1132,6 +1148,40 @@ mod tests {
assert_eq!(simplify(expr_plus), lit(2_i64));
}
+ #[test]
+ fn test_simplify_concat_ws_null_separator() {
+ fn build_concat_ws_expr(args: &[Expr]) -> Expr {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ConcatWithSeparator,
+ args: args.to_vec(),
+ }
+ }
+
+ let null = Expr::Literal(ScalarValue::Utf8(None));
+ // simple test
+ {
+ let expr = build_concat_ws_expr(&[null.clone(), col("c1"),
col("c2")]);
+ assert_eq!(simplify(expr), null);
+ }
+
Review Comment:
I wonder if we could support when arguments other than the first are null?
For example
```suggestion
// simple test
{
let expr = build_concat_ws_expr(&[null.clone(), col("c1"),
col("c2")]);
assert_eq!(simplify(expr), null);
}
// simple test with null further down the argument list
{
let expr = build_concat_ws_expr(&[col("c1"), null.clone(),
col("c2")]);
assert_eq!(simplify(expr), null);
}
```
I think the current code may only detect NULL in the first argument position
--
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]
