feat: add round function #322

vibhatha · 2022-09-08T14:58:00Z

This PR adds round function.

jvanstraten · 2022-09-08T15:21:01Z

Where did this come from? Based on #307 I thought we weren't going to do rounding to multiple. Rounding to some number of digits is exactly the same thing, just restricted to powers of 10.

That rounding argument is also really weird, there's no such thing as an optional value argument and using an integer for this is really weird. If we are going to do a round-to-multiple I feel like we should just reuse the existing rounding mode option to determine how to round.

Based on #307 I would expect simply round(f32) -> f32 and trunc(f32) -> f32 (and likewise for f64).

ianmcook · 2022-09-08T15:46:39Z

I think we will want to implement rounding and tie-breaking options similar to the ones in Acero's RoundMode.

vibhatha · 2022-09-08T15:48:41Z

Where did this come from? Based on #307 I thought we weren't going to do rounding to multiple. Rounding to some number of digits is exactly the same thing, just restricted to powers of 10.

That rounding argument is also really weird, there's no such thing as an optional value argument and using an integer for this is really weird. If we are going to do a round-to-multiple I feel like we should just reuse the existing rounding mode option to determine how to round.

Based on #307 I would expect simply round(f32) -> f32 and trunc(f32) -> f32 (and likewise for f64).

Referring to: https://github.com/substrait-io/substrait/pull/230/files#diff-b6317d19776a0381379a5980e1be0ffcde22082a034b891cb7a072fdbab32327

vibhatha · 2022-09-08T15:52:48Z

@jvanstraten I was referring to the SQL definition here.

I see your point on function argument. In Duckdb there is no such parameter. May be I will remove that option and also add the rounding options here too.

WDYT? cc @ianmcook

ianmcook · 2022-09-08T16:09:37Z

IIUC, I think we can just implement a rounding mode option, and make TRUNCATE one of the available modes, alongside the others like DOWN, UP, TOWARDS_ZERO, etc. Does that sound right to you @jvanstraten ?

jvanstraten · 2022-09-08T17:30:53Z

I figured we could just reuse the floating point rounding mode. I also generalized it to integers for quantile earlier. Wouldn't TRUNCATE and TOWARDS_ZERO be the same thing, anyway?

Note that dedicated ceil and floor functions already exist. I guess we should get rid of those if we go the option route.

ianmcook · 2022-09-08T18:00:16Z

Ah, yes, I think that all makes sense.

Since a lot of thought went into ensuring that Acero's RoundMode can represent the behavior of any other mainstream system's round function, I'll list out the options it provides and map them onto the options used in the Substrait functions that take a rounding argument. For more details about these Acero RoundModes, see the the round modes table here. To see an example showing the behavior of these different Acero RoundModes, look at this test.

DOWN - corresponds to Substrait's FLOOR
UP - corresponds to Substrait's CEILING
TOWARDS_ZERO - corresponds to Substrait's TRUNCATE
TOWARDS_INFINITY - no corresponding option in Substrait
HALF_DOWN - no corresponding option in Substrait
HALF_UP - no corresponding option in Substrait
HALF_TOWARDS_ZERO - no corresponding option in Substrait
HALF_TOWARDS_INFINITY - corresponds to Substrait's TIE_AWAY_FROM_ZERO
HALF_TO_EVEN - corresponds to Substrait's TIE_TO_EVEN
HALF_TO_ODD - no corresponding option in Substrait

So this raises some questions:

Should we add a Substrait option AWAY_FROM_ZERO? This corresponds to Acero's TOWARDS_INFINITY .
Should we add a Substrait option TIE_TOWARDS_NEGATIVE_INFINITY (or maybe TIE_DOWN to keep it shorter)? This corresponds to Acero's HALF_DOWN.
Should we add a Substrait option TIE_TOWARDS_POSITIVE_INFINITY (or maybe TIE_UP to keep it shorter)? This corresponds to Acero's HALF_UP.
Should we add a Substrait option TIE_TOWARDS_ZERO? This corresponds to Acero's HALF_TOWARDS_ZERO.
Should we add a Substrait option TIE_TO_ODD? This corresponds to Acero's HALF_TO_ODD.

jvanstraten · 2022-09-09T12:01:42Z

Hm. I guess that makes sense for round, and adding more variants is cheap. I don't think it makes sense to do it for all floating point functions though, since IEEE is pretty explicit about what the possible rounding modes are.

vibhatha · 2022-09-19T06:42:30Z

cc @jvanstraten I added the round options. Does it reflect what you expected?

jvanstraten · 2022-09-19T13:35:23Z

No, because you didn't add the rounding modes @ianmcook was talking about, and the supposedly-optional function argument still exists.

vibhatha · 2022-09-21T13:45:12Z

@jvanstraten I added other options. Should we create documentation for this too? In this PR itself or a separate PR?

jvanstraten · 2022-09-21T16:01:08Z

It should just be in the description of the function in this case; this option argument is basically specific to round. The rounding mode should probably be a required argument, too, since it makes little sense for a producer to allow a consumer to round however it wants for a function that is specifically about rounding (unlike the usual floating point rounding mode).

vibhatha · 2022-09-21T16:43:00Z

@jvanstraten I had the doubt though. I will update it.

CLAassistant · 2022-10-06T23:47:08Z

All committers have signed the CLA.

CLAassistant · 2022-10-06T23:47:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

westonpace

@vibhatha when you get a moment can you update this to the new format for optional arguments?

vibhatha · 2022-12-02T00:42:28Z

@vibhatha when you get a moment can you update this to the new format for optional arguments?

sure @westonpace, thanks for the ping. I will update it.

extensions/functions_rounding.yaml

…#322

westonpace · 2023-01-30T17:34:06Z

@vibhatha can you update this when you get a chance?

vibhatha · 2023-01-30T17:35:12Z

@westonpace will update it tomorrow. Sorry about the delay.

vibhatha · 2023-02-07T03:38:36Z

@westonpace sorry about the long wait. I updated the PR, although I see branch is out-of-date with base branch. I have rebased and did --force push.

westonpace

Thanks for the update. It still says out of date but a rebase should have fixed that. Did you make sure you had updated your local main branch before rebasing?

westonpace · 2023-02-07T14:22:16Z

extensions/functions_rounding.yaml

+              to round it. For floating point numbers, it specifies the IEEE
+              754 rounding mode (as it does for all other floating point
+              operations). For integer types:


You removed this test below (in the fp64/i32 kernel). Can you also remove it here (in the fp32/i32 kernel)

extensions/functions_rounding.yaml

vibhatha · 2023-02-07T14:56:53Z

Thanks for the update. It still says out of date but a rebase should have fixed that. Did you make sure you had updated your local main branch before rebasing?

main and round_func branches are rebased with the upstream/main. Not sure what is going on here?

vibhatha · 2023-02-08T00:01:23Z

@westonpace I think now the branch is in sync with the base branch.

westonpace

Thanks for adding integer kernels. I think the description will need to be slightly different for these kernels.

extensions/functions_rounding.yaml

westonpace · 2023-02-08T22:18:32Z

extensions/functions_rounding.yaml

+                - TIE_TO_EVEN: round to nearest value; if exactly halfway, tie
+                  to the even option.
+                - TIE_AWAY_FROM_ZERO: round to nearest value; if exactly
+                  halfway, tie away from zero.
+                - TRUNCATE: always round toward zero.
+                - CEILING: always round toward positive infinity.
+                - FLOOR: always round toward negative infinity.
+                - AWAY_FROM_ZERO: round negative values with FLOOR rule, round positive values with CEILING rule
+                - TIE_DOWN: round ties with FLOOR rule
+                - TIE_UP: round ties with CEILING rule
+                - TIE_TOWARDS_ZERO: round ties with TRUNCATE rule
+                - TIE_TO_ODD: round to nearest value; if exactly halfway, tie
+                  to the odd option.


What does TIE_TO_EVEN and TIE_TO_ODD mean for negative values of s?

CC @ianmcook

Ref: https://docs.oracle.com/cd/E57185_01/AGOHP/apbs03s18.html

It is not quite clear from this definition either: https://dev.mysql.com/doc/refman/5.7/en/mathematical-functions.html#function_round
🤔

Ok. I did some investigation. As an example, consider 15 when s=-1. The two choices are 10 and 20 which are both even. However, when we drop the trailing 0's so the choices are 1 and 2 and 2 is the even one so we get 20.

I don't think we need to clarify. I don't think we can easily do so without writing a novel.

I played with Mysql a bit, but with the round function, can we specify such options?

westonpace

Alright, I think this looks good. @EpsilonPrime did you want to take another look?

vibhatha · 2023-02-09T15:51:49Z

@westonpace a CI is failing and this happened a few times. Seems like unrelated, though not very sure about it.

EpsilonPrime

LGTM

westonpace · 2023-02-09T17:39:24Z

@westonpace a CI is failing and this happened a few times. Seems like unrelated, though not very sure about it.

Unrelated to this PR. I created #439 which should address it.

vibhatha marked this pull request as ready for review September 8, 2022 14:59

vibhatha force-pushed the round_func branch from e2698a9 to 94fcb33 Compare September 8, 2022 14:59

jvanstraten changed the title ~~feat: Adding round function~~ feat: add round function Sep 8, 2022

vibhatha force-pushed the round_func branch from 94fcb33 to f6af6fe Compare September 19, 2022 06:41

vibhatha closed this Sep 19, 2022

vibhatha reopened this Sep 19, 2022

vibhatha force-pushed the round_func branch from 75f55b2 to 3e47415 Compare September 21, 2022 13:38

ianmcook mentioned this pull request Nov 28, 2022

feat: add functions for arithmetic, rounding, logarithmic, and string transformations #230

Closed

westonpace requested changes Dec 1, 2022

View reviewed changes

EpsilonPrime reviewed Dec 13, 2022

View reviewed changes

extensions/functions_rounding.yaml Outdated Show resolved Hide resolved

EpsilonPrime reviewed Dec 13, 2022

View reviewed changes

extensions/functions_rounding.yaml Outdated Show resolved Hide resolved

vibhatha force-pushed the round_func branch 2 times, most recently from aa17207 to e2c9f4d Compare December 13, 2022 03:59

vibhatha requested a review from westonpace December 13, 2022 03:59

EpsilonPrime reviewed Jan 6, 2023

View reviewed changes

extensions/functions_rounding.yaml Show resolved Hide resolved

EpsilonPrime added a commit to EpsilonPrime/arrow that referenced this pull request Jan 12, 2023

Add Subtrait->Acero mapping for round to match substrait-io/substrait…

39d4bfb

…#322

asfimport mentioned this pull request Dec 20, 2022

Add support for Substrait round expression apache/arrow#33588

Closed

thisisnic mentioned this pull request Jan 31, 2023

Implement binding for round() voltrondata/substrait-r#246

Merged

jcrist mentioned this pull request Jan 31, 2023

Support round ibis-project/ibis-substrait#493

Closed

westonpace requested changes Feb 7, 2023

View reviewed changes

vibhatha added 7 commits February 8, 2023 05:19

feat(round_func): squash merge

fe0ae0b

fix(format): options format updated

d850ca2

fix(lint): formatting options

dff15aa

fix(functions): remove functions from round definition:

2eae65e

fix(description): adding rounding mode description

665a7f2

fix(reviews): removing IEEE 754 rounding mode docs

17955d3

fix(reviews): adding i8, i16, i32 and i64 for x

d66fd7b

vibhatha force-pushed the round_func branch from 633b734 to d66fd7b Compare February 7, 2023 23:49

westonpace requested changes Feb 8, 2023

View reviewed changes

fix(round_desc): for integer values

138786a

westonpace approved these changes Feb 9, 2023

View reviewed changes

EpsilonPrime reviewed Feb 9, 2023

View reviewed changes

westonpace closed this Feb 9, 2023

westonpace reopened this Feb 9, 2023

westonpace merged commit 57121c8 into substrait-io:main Feb 9, 2023

This was referenced Mar 8, 2023

Support Round Function Ibis - DuckDB substrait-io/duckdb-substrait-extension#49

Closed

Support temporal functions duckdb - ibis substrait-io/duckdb-substrait-extension#50

Closed

feat: add round function #322

feat: add round function #322

Conversation

vibhatha commented Sep 8, 2022

jvanstraten commented Sep 8, 2022

ianmcook commented Sep 8, 2022 • edited Loading

vibhatha commented Sep 8, 2022

vibhatha commented Sep 8, 2022

ianmcook commented Sep 8, 2022 • edited Loading

jvanstraten commented Sep 8, 2022

ianmcook commented Sep 8, 2022 • edited Loading

jvanstraten commented Sep 9, 2022

vibhatha commented Sep 19, 2022

jvanstraten commented Sep 19, 2022

vibhatha commented Sep 21, 2022

jvanstraten commented Sep 21, 2022

vibhatha commented Sep 21, 2022

CLAassistant commented Oct 6, 2022 • edited Loading

CLAassistant commented Oct 6, 2022

westonpace left a comment

Choose a reason for hiding this comment

vibhatha commented Dec 2, 2022

westonpace commented Jan 30, 2023

vibhatha commented Jan 30, 2023

vibhatha commented Feb 7, 2023

westonpace left a comment

Choose a reason for hiding this comment

westonpace Feb 7, 2023

Choose a reason for hiding this comment

vibhatha commented Feb 7, 2023

vibhatha commented Feb 8, 2023

westonpace left a comment

Choose a reason for hiding this comment

westonpace Feb 8, 2023

Choose a reason for hiding this comment

vibhatha Feb 8, 2023

Choose a reason for hiding this comment

vibhatha Feb 8, 2023

Choose a reason for hiding this comment

westonpace Feb 9, 2023

Choose a reason for hiding this comment

westonpace Feb 9, 2023

Choose a reason for hiding this comment

vibhatha Feb 9, 2023

Choose a reason for hiding this comment

westonpace left a comment

Choose a reason for hiding this comment

vibhatha commented Feb 9, 2023

EpsilonPrime left a comment

Choose a reason for hiding this comment

westonpace commented Feb 9, 2023

ianmcook commented Sep 8, 2022 •

edited

Loading

ianmcook commented Sep 8, 2022 •

edited

Loading

ianmcook commented Sep 8, 2022 •

edited

Loading

CLAassistant commented Oct 6, 2022 •

edited

Loading