Unfold the `round` function in logical plan if it has 2 arguments by HaoYang670 · Pull Request #4234 · apache/datafusion

HaoYang670 · 2022-11-16T07:06:35Z

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #4191.

Rationale for this change

Before the change

❯ select round(12.45, -2), round(12.45, -1), round(12.45), round(12.45, 0), round(12.45, 1), round(12.45, 2);
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
| round(Float64(12.45),Int64(-2)) | round(Float64(12.45),Int64(-1)) | round(Float64(12.45)) | round(Float64(12.45),Int64(0)) | round(Float64(12.45),Int64(1)) | round(Float64(12.45),Int64(2)) |
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
| 12                              | 12                              | 12                    | 12                             | 12                             | 12                             |
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
1 row in set. Query took 0.019 seconds.

After the change

❯ select round(12.45, -2), round(12.45, -1), round(12.45), round(12.45, 0), round(12.45, 1), round(12.45, 2);
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
| round(Float64(12.45),Int64(-2)) | round(Float64(12.45),Int64(-1)) | round(Float64(12.45)) | round(Float64(12.45),Int64(0)) | round(Float64(12.45),Int64(1)) | round(Float64(12.45),Int64(2)) |
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
| 0                               | 10                              | 12                    | 12                             | 12.5                           | 12.45                          |
+---------------------------------+---------------------------------+-----------------------+--------------------------------+--------------------------------+--------------------------------+
1 row in set. Query took 0.008 seconds.

What changes are included in this PR?

Unfold the expression round(source, n) as round(source * pow(10, n)) / pow(10, n) in the logical plan.
Here are some reasons that I don't implement round(source, n) in the physical expression, but in logical stage:

rust doesn't provide the round function with 2 arguments
We can do some constant folding in the logical stage if round contains literal arguments

Are these changes tested?

Yes.

Are there any user-facing changes?

Signed-off-by: remzi <13716567376yh@gmail.com>

…_2nd_args

HaoYang670 · 2022-11-16T07:29:44Z

+                    println!("I am here, source = {:?}, n = {:?}", source, n);
+                    let exp = Box::new(power(
+                        lit(10.0),
+                        Expr::Cast(Cast::new(n.clone().into(), DataType::Float64)),


One problem here is that we will always convert the data type to Float64 to adapt the power function

HaoYang670 · 2022-11-16T08:07:21Z

Also, I didn't want to put this logic in the optimizer. Because it doesn't optimize the plan, but convert some syntax sugar. I plan to create a new stage calling "desugar_expression" and put it in front of the optimizer, if it is necessary.

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 · 2022-11-16T08:39:29Z

+    let sql = "SELECT round(col_f32, col_int32) as col from test";
+    let plan = test_sql(sql)?;
+    let expected =
+        "Projection: round(CAST(test.col_f32 AS Float64) * power(Float64(10), CAST(CAST(test.col_int32 AS Int64) AS Float64))CAST(CAST(test.col_int32 AS Int64) AS Float64)CAST(test.col_int32 AS Int64)test.col_int32Float64(10) AS power(Float64(10),test.col_int32)) / power(Float64(10), CAST(CAST(test.col_int32 AS Int64) AS Float64))CAST(CAST(test.col_int32 AS Int64) AS Float64)CAST(test.col_int32 AS Int64)test.col_int32Float64(10) AS power(Float64(10),test.col_int32) AS col\


The lots of Cast here is a little weird, I am looking into it.

Possibly related to #3786 ?

HaoYang670 · 2022-11-16T08:40:30Z

+    let sql = "SELECT round(col_f32, 2) as col from test";
+    let plan = test_sql(sql)?;
+    let expected =
+        "Projection: round(CAST(test.col_f32 AS Float64) * Float64(100)) / Float64(100) AS col\


We can benefit from the expression simplification when one of the argument is a constant

HaoYang670 · 2022-11-16T08:45:55Z

@andygrove Could you please help to review?

andygrove · 2022-11-16T17:15:50Z

+            // unfold the expression round(source, n)
+            Expr::ScalarFunction {
+                fun: BuiltinScalarFunction::Round,
+                args,


I would prefer that we don't re-write this in the logical plan to work around the fact that we don't currently support this in the physical plan. This makes the logical plan harder to use for other query engines, such as Dask SQL, that are using DataFusion for SQL query planning.

I suggest for this PR that we throw an error in the physical query planner if we see round with two arguments. This would be better than producing incorrect results. We can implement the functionality in the physical plan in a separate PR.

This makes the logical plan harder to use for other query engines

Why is that? Could you please give an example?

We can implement the functionality in the physical plan in a separate PR.

The advantages of implementing in physical plan are:

much straight forward and easy to debug.

we would benefit from arrow-rs, if arrow-rs could support round(source, n) in the future.

The disadvantages of implementing in physical plan are:

Rust only support the round function with one argument. If we want to implement in physical plan, we have to assemble some arrow compute kernel together, which the logic is same as what I've done in the physical plan.

We can't optimize the expression if the second function is a constant, which we have to add additional optimize rules.

The advantages of implementing in logical plan are:

Reuse other code to achieve the functionality of round(source, n), so the implementation could be much easier.

We can reuse our expression optimizer to do optimizations, for example when the 2nd argument is a constant.

Flexible. If we plan to change the parameter type of round from Float, to Decimal, the change can be very easy.

Make logical stage and physical stage independent. We can achieve complex functionality in the SQL query by unfolding and rewriting expressions (or plans) in the logical stage and we only need to maintain a reduced set of physical expressions (or plans).

The disadvantages of implementing in logical plan are:

Put the implementation in the logical optimizer isn't a good solution, we should unfold the expression before doing any optimization.

the query plan isn't user-friendly to read because round is unfolded. But I think this is not a big deal, as the optimized code is always ugly to read. Users can also use explain verbose to find what has happened.

After weighing the pros and cons, I prefer to implement in logical plan. If the rust lib or arrow-rs could support round(source, n), in the future, we can change it.

Why is that? Could you please give an example?

Sure. Taking Dask SQL as an example, we would want to translate round(a, b) to a call to a Python function to perform the rounding (perhaps in numpy). With this PR, the logical plan now has a complex expression that is specific to the DataFusion implementation. Dask SQL would have to undo the pow logic here to get back to the original parameters to round so that it can pass them to numpy.

Projection: round(CAST(test.column_1 AS Float64) * Float64(0.01)) / Float64(0.01) AS round(test.column_1,Int64(-2))

With this PR, the logical plan now has a complex expression that is specific to the DataFusion implementation. Dask SQL would have to undo the pow logic here to get back to the original parameters to round so that it can pass them to numpy.

If the dask-sql has a physical execution to support round(source, n), this could be a problem. 🤔

the logical plan now has a complex expression that is specific to the DataFusion implementation

This is a little weird for me that the Datafusion logical plan should not be specific to Datafusion physical plan.
If this is the truth, why don't we seperate the Datafusion to be two projects?

alamb · 2023-11-28T20:09:58Z

Closing as this PR is over a year old. Please feel free to reopen it / rebase it if you plan to keep working on it

unfold round in logical plan

1f8cf64

Signed-off-by: remzi <13716567376yh@gmail.com>

github-actions Bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Nov 16, 2022

HaoYang670 marked this pull request as draft November 16, 2022 07:06

Merge remote-tracking branch 'upstream/master' into 4191_round_ignore…

caee1b2

…_2nd_args

HaoYang670 commented Nov 16, 2022

View reviewed changes

add test

4acdcc0

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 marked this pull request as ready for review November 16, 2022 08:30

HaoYang670 changed the title ~~[WIP] Unfold the round function in logical plan if it has 2 arguments~~ Unfold the round function in logical plan if it has 2 arguments Nov 16, 2022

remove debug line

f162296

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 commented Nov 16, 2022

View reviewed changes

andygrove reviewed Nov 16, 2022

View reviewed changes

HaoYang670 marked this pull request as draft November 18, 2022 09:11

HaoYang670 mentioned this pull request Jan 22, 2023

scalar function round ignores second parameter #4191

Closed

alamb closed this Nov 28, 2023

Navigation Menu

Repository navigation

Unfold the `round` function in logical plan if it has 2 arguments - #4234

Unfold the `round` function in logical plan if it has 2 arguments#4234
HaoYang670 wants to merge 4 commits into
apache:mainfrom
HaoYang670:4191_round_ignore_2nd_args

HaoYang670 commented Nov 16, 2022 •

edited

Loading

Uh oh!

HaoYang670 Nov 16, 2022

Uh oh!

HaoYang670 commented Nov 16, 2022

Uh oh!

HaoYang670 Nov 16, 2022

Uh oh!

andygrove Nov 16, 2022

Uh oh!

HaoYang670 Nov 16, 2022

Uh oh!

HaoYang670 commented Nov 16, 2022

Uh oh!

andygrove Nov 16, 2022

Uh oh!

HaoYang670 Nov 17, 2022 •

edited

Loading

Uh oh!

andygrove Nov 17, 2022

Uh oh!

HaoYang670 Nov 18, 2022 •

edited

Loading

Uh oh!

alamb commented Nov 28, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

HaoYang670 commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

Before the change

After the change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

HaoYang670 Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

HaoYang670 commented Nov 16, 2022

Uh oh!

HaoYang670 Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

andygrove Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

HaoYang670 Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

HaoYang670 commented Nov 16, 2022

Uh oh!

andygrove Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

HaoYang670 Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

HaoYang670 Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 28, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HaoYang670 commented Nov 16, 2022 •

edited

Loading

HaoYang670 Nov 17, 2022 •

edited

Loading

HaoYang670 Nov 18, 2022 •

edited

Loading