Convert Binary Operator `StringConcat` to Function for `array_concat`, `array_append` and `array_prepend` #8636

jayzhan211 · 2023-12-23T03:04:56Z

Which issue does this PR close?

Part of #8506

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211 · 2023-12-23T03:05:55Z

datafusion/sql/src/expr/mod.rs

+
+                    // Convert `Array StringConcat Array` to ScalarFunction::ArrayConcat
+                    let expr = match (op, &left, &right) {
+                        // Chain concat operator (a || b) || array,


This is really long. :(
Not yet find a nicer way

It seems like the rule to convert Operator::StringConcat ==> ArrayConcat depends on the type of the arguments and this code is effectively rewriting the originally created Expr tree.

What do you think about putting this logic in an analyzer rule: https://github.com/apache/arrow-datafusion/tree/main/datafusion/optimizer/src/analyzer?

alamb · 2023-12-28T20:38:44Z

Thank you for this @jayzhan211 -- I plan to review it carefully tomorrow

alamb

Thanks @jayzhan211 -- I found the rationale by @viirya #8496 (comment) makes sense.

Given the idea is to make the logic for function / operator aliasing easier to find / centralized in the codebase, perhaps a better place for this logic could be an analyzer rule (for example OperatorsToFunction) that would be easier to find and easier to document what it was doing

alamb · 2023-12-31T13:43:58Z

datafusion/sql/src/expr/mod.rs

+
+                    // Convert `Array StringConcat Array` to ScalarFunction::ArrayConcat
+                    let expr = match (op, &left, &right) {
+                        // Chain concat operator (a || b) || array,


It seems like the rule to convert Operator::StringConcat ==> ArrayConcat depends on the type of the arguments and this code is effectively rewriting the originally created Expr tree.

What do you think about putting this logic in an analyzer rule: https://github.com/apache/arrow-datafusion/tree/main/datafusion/optimizer/src/analyzer?

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-01-02T12:57:17Z

datafusion/optimizer/src/analyzer/rewrite_expr.rs

+fn analyze_internal(plan: LogicalPlan) -> Result<LogicalPlan> {
+    // OperatorToFunction is only applied to Projection
+    match plan {
+        LogicalPlan::Projection(_) => {}


Based on test, I found they are all project plan with inputs.len() 1. Not sure is this assumption correct or not

This will not catch uses of operators in all places (like LogicalPlan::Filter)

I think you can follow the model of https://github.com/apache/arrow-datafusion/blob/f4233a92761e9144b8747e66b95bf0b3f82464b8/datafusion/optimizer/src/analyzer/type_coercion.rs#L74-L122 and call LogicalPlan::expressions() to get all the expressions in a LogicalPlan node, rewrite them appropriately, and then call LogicaPlan::new_with_exprs to get the rewritten node

let new_expr = plan .expressions() .into_iter() .map(|expr| { // ensure aggregate names don't change: // https://github.com/apache/arrow-datafusion/issues/3555 rewrite_preserving_name(expr, &mut expr_rewrite) }) .collect::<Result<Vec<_>>>()?;

I did follow the code in type coercion at the first place, but I'm not sure whether we need these for operatorToFunction since I did find any test cases

It seems column wise cases cover it.

jayzhan211 · 2024-01-02T12:57:25Z

datafusion/optimizer/src/analyzer/rewrite_expr.rs

+        .collect::<Result<Vec<_>>>()?;
+
+    // Not found cases that inputs more than one
+    assert_eq!(plan.inputs().len(), 1);


Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-01-02T14:28:51Z

cargo test (win64) has been run over 1hr ... Since cargo test (mac) pass, I think that may be a bug in CI.

alamb · 2024-01-02T15:38:56Z

cargo test (win64) has been run over 1hr ... Since cargo test (mac) pass, I think that may be a bug in CI.

I think @Jefffrey noticed the same thing here: #8696

alamb

Thank you @jayzhan211 -- this is lookingvery close -- I think the recursion needs a small tweak to cover all the types of LogicalPlans but otherwise 👍

alamb · 2024-01-02T15:42:54Z

datafusion/optimizer/src/analyzer/rewrite_expr.rs

+fn analyze_internal(plan: LogicalPlan) -> Result<LogicalPlan> {
+    // OperatorToFunction is only applied to Projection
+    match plan {
+        LogicalPlan::Projection(_) => {}


This will not catch uses of operators in all places (like LogicalPlan::Filter)

I think you can follow the model of https://github.com/apache/arrow-datafusion/blob/f4233a92761e9144b8747e66b95bf0b3f82464b8/datafusion/optimizer/src/analyzer/type_coercion.rs#L74-L122 and call LogicalPlan::expressions() to get all the expressions in a LogicalPlan node, rewrite them appropriately, and then call LogicaPlan::new_with_exprs to get the rewritten node

let new_expr = plan .expressions() .into_iter() .map(|expr| { // ensure aggregate names don't change: // https://github.com/apache/arrow-datafusion/issues/3555 rewrite_preserving_name(expr, &mut expr_rewrite) }) .collect::<Result<Vec<_>>>()?;

datafusion/optimizer/src/analyzer/rewrite_expr.rs

Signed-off-by: jayzhan211 <[email protected]>

Co-authored-by: Andrew Lamb <[email protected]>

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-01-04T00:07:12Z

datafusion/optimizer/src/analyzer/rewrite_expr.rs

+    }
+}
+
+fn analyze_internal(plan: &LogicalPlan) -> Result<LogicalPlan> {


external_schema is not used here.

alamb

Looks good to me -- thank you for sticking with it @jayzhan211

alamb · 2024-01-04T22:39:15Z

cc @Veeupup as I think you were working on something similar

alamb · 2024-01-05T22:14:58Z

Thanks again @jayzhan211

github-actions bot added sql SQL Planner physical-expr Physical Expressions labels Dec 23, 2023

jayzhan211 commented Dec 23, 2023

View reviewed changes

jayzhan211 mentioned this pull request Dec 23, 2023

Convert list operator to function in sql_expr_to_logical_expr #8506

Open

github-actions bot added the logical-expr Logical plan and expressions label Dec 23, 2023

jayzhan211 changed the title ~~Convert Binary Operator StringConcat to Function array_concat, array_append and array_prepend~~ Convert Binary Operator StringConcat to Function for array_concat, array_append and array_prepend Dec 23, 2023

jayzhan211 marked this pull request as ready for review December 23, 2023 03:57

jayzhan211 marked this pull request as draft December 28, 2023 00:56

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Dec 28, 2023

jayzhan211 marked this pull request as ready for review December 28, 2023 01:29

alamb mentioned this pull request Dec 28, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 25, 2023 #8655

Closed

7 tasks

alamb reviewed Dec 31, 2023

View reviewed changes

jayzhan211 marked this pull request as draft January 1, 2024 23:48

jayzhan211 added 4 commits January 2, 2024 08:21

reuse function for string concat

bffd825

Signed-off-by: jayzhan211 <[email protected]>

remove casting in string concat

e7cfcce

Signed-off-by: jayzhan211 <[email protected]>

add test

0c18f78

Signed-off-by: jayzhan211 <[email protected]>

operator to function rewrite

6201176

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 force-pushed the list-concat-rewrite branch from 9131096 to 6201176 Compare January 2, 2024 12:53

github-actions bot added the optimizer Optimizer rules label Jan 2, 2024

jayzhan211 commented Jan 2, 2024

View reviewed changes

fix explain

69df2b6

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as ready for review January 2, 2024 14:19

jayzhan211 requested a review from alamb January 2, 2024 14:19

alamb reviewed Jan 2, 2024

View reviewed changes

alamb mentioned this pull request Jan 2, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 1, 2024 #8704

Closed

9 tasks

add more test

77386b8

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as draft January 3, 2024 01:11

jayzhan211 and others added 5 commits January 3, 2024 09:30

add column cases

a905394

Signed-off-by: jayzhan211 <[email protected]>

cleanup

e4c83ab

Signed-off-by: jayzhan211 <[email protected]>

presever name

1ef0e50

Signed-off-by: jayzhan211 <[email protected]>

Update datafusion/optimizer/src/analyzer/rewrite_expr.rs

111299b

Co-authored-by: Andrew Lamb <[email protected]>

rename

6a0d782

Signed-off-by: jayzhan211 <[email protected]>

This comment was marked as outdated.

Sign in to view

jayzhan211 marked this pull request as ready for review January 3, 2024 12:56

jayzhan211 requested a review from alamb January 3, 2024 12:56

jayzhan211 commented Jan 4, 2024

View reviewed changes

alamb approved these changes Jan 4, 2024

View reviewed changes

alamb merged commit 4e4059a into apache:main Jan 5, 2024
22 checks passed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Binary Operator `StringConcat` to Function for `array_concat`, `array_append` and `array_prepend` #8636

Convert Binary Operator `StringConcat` to Function for `array_concat`, `array_append` and `array_prepend` #8636

jayzhan211 commented Dec 23, 2023

jayzhan211 Dec 23, 2023

alamb Dec 31, 2023

alamb commented Dec 28, 2023

alamb left a comment

alamb Dec 31, 2023

jayzhan211 Jan 2, 2024

alamb Jan 2, 2024

jayzhan211 Jan 2, 2024

jayzhan211 Jan 3, 2024

jayzhan211 Jan 2, 2024

jayzhan211 commented Jan 2, 2024 •

edited

Loading

alamb commented Jan 2, 2024

alamb left a comment

alamb Jan 2, 2024

This comment was marked as outdated.

jayzhan211 Jan 4, 2024

alamb left a comment

alamb commented Jan 4, 2024

alamb commented Jan 5, 2024

Convert Binary Operator StringConcat to Function for array_concat, array_append and array_prepend #8636

Convert Binary Operator StringConcat to Function for array_concat, array_append and array_prepend #8636

Conversation

jayzhan211 commented Dec 23, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 28, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Jan 2, 2024 • edited Loading

alamb commented Jan 2, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jan 4, 2024

alamb commented Jan 5, 2024

Convert Binary Operator `StringConcat` to Function for `array_concat`, `array_append` and `array_prepend` #8636

Convert Binary Operator `StringConcat` to Function for `array_concat`, `array_append` and `array_prepend` #8636

jayzhan211 commented Jan 2, 2024 •

edited

Loading