Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scalar function round ignores second parameter #4191

Closed
andygrove opened this issue Nov 12, 2022 · 3 comments · Fixed by #5807
Closed

scalar function round ignores second parameter #4191

andygrove opened this issue Nov 12, 2022 · 3 comments · Fixed by #5807
Labels
bug Something isn't working
Milestone

Comments

@andygrove
Copy link
Member

andygrove commented Nov 12, 2022

Describe the bug

round accepts two arguments, but is also defined as a unary function. The second parameter is ignored.

❯ select round(3.141592, 0);
+-----------------------------------+
| round(Float64(3.141592),Int64(0)) |
+-----------------------------------+
| 3                                 |
+-----------------------------------+
1 row in set. Query took 0.000 seconds.

❯ select round(3.141592, -2);
+------------------------------------+
| round(Float64(3.141592),Int64(-2)) |
+------------------------------------+
| 3                                  |
+------------------------------------+
1 row in set. Query took 0.000 seconds.

❯ select round(3.141592, 2);
+-----------------------------------+
| round(Float64(3.141592),Int64(2)) |
+-----------------------------------+
| 3                                 |
+-----------------------------------+
1 row in set. Query took 0.000 seconds.

To Reproduce
See above

Expected behavior
I expect the behavior to be consistent with Postgres round(v numeric, s int). User guide will also need updating.

Additional context
Related to #2420

@andygrove andygrove added the bug Something isn't working label Nov 12, 2022
@andygrove andygrove added this to the 15.0.0 milestone Nov 12, 2022
@HaoYang670
Copy link
Contributor

HaoYang670 commented Nov 15, 2022

Not sure, but I wonder if we could support this in logical phase.
Given the following 2 lemmas:

1. forall source: FloatArray, round(source) = round(source, 0)
2. forall source: FloatArray n: IntArray | IntScalar,
        round(source, n) = div(
                              round(mul(
                                      source, 
                                      pow(10, n))), 
                              pow(10, n))

we can unfold the round function in the logical plan stage and don't need to modify the physical stage.

One concern here is that Decimal type might be better than Float type because we've found some corner cases in floating point multiplication and division: #4072

@DDtKey
Copy link
Contributor

DDtKey commented Jan 22, 2023

Just wondering if anyone is working on it? It's relevant for current version. I see the same behaviour

@HaoYang670
Copy link
Contributor

I've pushed a draft PR (#4234) to unfold the round function with 2 args in logical plan.
But it seems like not a good solution because it would reduce the extensibility of the logical optimizer.

Based on Andy's comment (#4234 (comment)) we should either support this or return an error in the physical plan.

Just wondering if anyone is working on it?

No, I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants