sql: improve the ROUND and EXP functions. #8822

knz · 2016-08-25T14:42:54Z

Prior to this patch the computation would take too long or too much
memory if EXP was given an argument too large or if the 2nd ROUND
argument was negative with a large absolute value.

This patch addresses this issue by limiting the range of acceptable
arguments (1024 for EXP, anything that causes more than 2000 extra "0"
digits to be appended on the right for ROUND).

It improves both functions as follows:

a negative ROUND scale is now supported as in PostgreSQL and Oracle;
this rounds to positions on the left of the decimal separator.
the ROUND function for floats is now performed on floats directly,
instead of using Printf/ParseFloat as previously (faster).
the ROUND function for decimals is now faster if the 2nd argument is
negative and its absolute value is larger than the number of digits
in the 1st argument on the left of its decimal separator.
ROUND for both floats and decimal now use half to even rounding,
also called unbiased rounding or bankers' rounding.

Fixes #8633.

This change is

knz · 2016-08-25T14:57:51Z

sql/parser/builtins.go

+	// Compute the size of a Word in bits.
+	_m       = ^big.Word(0)
+	_logS    = _m>>8&1 + _m>>16&1 + _m>>32&1
+	wordSize = 8 * (1 << _logS)


Any suggestion as to how to implement this without having go vet bark on _m >> 32 like it currently does?

maddyblue · 2016-08-25T18:38:48Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

sql/parser/builtins.go, line 1436 [r1] (raw file):

Previously, knz (kena) wrote…

Any suggestion as to how to implement this without having go vet bark on _m >> 32 like it currently does?

We're lucky: I wrote that vet test. `_m = int64(^big.Word(0))` will force _m to be an int64 instead of int. int can be 32-bits on some systems, which is why vet is complaining. Crazy that it's the same code in math/big. I guess they ignore some vet warnings?

Comments from Reviewable

nvanbenschoten · 2016-08-25T18:47:40Z

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

sql/parser/builtins.go, line 52 [r1] (raw file):

  errEmptyInputString = errors.New("the input string must not be empty")
  errAbsOfMinInt64    = errors.New("abs of min integer value (-9223372036854775808) not defined")
  errRoundTooLow      = errors.New("rounding would extend value by more than 2000 decimals")

s/2000 decimals/2000 digits?

sql/parser/builtins.go, line 53 [r1] (raw file):

  errAbsOfMinInt64    = errors.New("abs of min integer value (-9223372036854775808) not defined")
  errRoundTooLow      = errors.New("rounding would extend value by more than 2000 decimals")
  errArgTooBig        = errors.New("argument value is too large")

I wonder if this should say the limit to be more helpful.

sql/parser/builtins.go, line 1406 [r1] (raw file):

  pow := math.Pow(10, float64(n))

  if pow == 0 {

my personal preference would be a switch statement to handle all edge cases here, but it's up to you

sql/parser/builtins.go, line 1419 [r1] (raw file):

  v, frac := math.Modf(x * pow)
  if x > 0.0 {
      if frac > 0.5 || (frac == 0.5 && uint64(v)%2 != 0) {

You might want to add a comment about the (frac == 0.5 && uint64(v)%2 != 0) here and below. Something about SQL rounding and how evens are rounded up, etc. I'm not sure if there's a specific name for this, but I've heard it called "unbiased rounding".

sql/parser/builtins.go, line 1436 [r1] (raw file):

Previously, knz (kena) wrote…

Any suggestion as to how to implement this without having go vet bark on _m >> 32 like it currently does?

We have a lot of this in `util/encoding/decimal.go`. You can just use `int(unsafe.Sizeof(big.Word(0)))`, which has the same effect.

sql/parser/builtins.go, line 1444 [r1] (raw file):

  if n > curScale+2000 {
      // If we let the decimal value grow too many decimals, the server
      // could explode (#8633).

sql/parser/builtins.go, line 1453 [r1] (raw file):

  upperDigitsLeft := float64(curScale) - float64(upperCurDigits)*scaleRatio
  if n >= int64(upperDigitsLeft)-1 {
      // This is an optimization. When the rounding scale is definitely

I would change this wording or reverse the inequality. You're talking about definitely larger here, so it made me think you would enter this block for the optimization and that n was the rounding scale. Instead, we avoid this block for the optimization.

sql/parser/builtins.go, line 1464 [r1] (raw file):

  // The computation of Exp is separated in the decimal module by
  // computing the exponents on the left and right of the decimal
  // separator.  The computation on the right is bounded by

s/ / / (two spaces -> one)

sql/testdata/builtin_function, line 686 [r1] (raw file):


query error rounding would extend value by more than 2000 decimals
SELECT round(1::decimal, 3000)

We should add a test for the errArgTooBig case.

Comments from Reviewable

maddyblue · 2016-08-25T18:52:39Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

sql/parser/builtins.go, line 1419 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You might want to add a comment about the (frac == 0.5 && uint64(v)%2 != 0) here and below. Something about SQL rounding and how evens are rounded up, etc. I'm not sure if there's a specific name for this, but I've heard it called "unbiased rounding".

Also called bankers' rounding.

Comments from Reviewable

knz · 2016-08-25T22:10:34Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

sql/parser/builtins.go, line 52 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/2000 decimals/2000 digits?

Done.

sql/parser/builtins.go, line 53 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I wonder if this should say the limit to be more helpful.

I'm not yet comfortable with the specific limit I chose so I'd rather not advertise it for now.

sql/parser/builtins.go, line 1406 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

my personal preference would be a switch statement to handle all edge cases here, but it's up to you

I'm not comfortable using switch here because switch suggests the conditions are mutually exclusive and thus that the ordering of tests is indeterminate. Here the conditions have to be evaluated in sequence.

sql/parser/builtins.go, line 1419 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Also called bankers' rounding.

Done. Also changed roundDecimal to use the same for consistency.

sql/parser/builtins.go, line 1436 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We have a lot of this in util/encoding/decimal.go. You can just use int(unsafe.Sizeof(big.Word(0))), which has the same effect.

Thanks for the tip! I'm using the Sizeof solution now.

sql/parser/builtins.go, line 1444 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

🔥 🔥 🔥

sql/parser/builtins.go, line 1453 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I would change this wording or reverse the inequality. You're talking about definitely larger here, so it made me think you would enter this block for the optimization and that n was the rounding scale. Instead, we avoid this block for the optimization.

Done.

sql/parser/builtins.go, line 1464 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/ / / (two spaces -> one)

Done.

sql/testdata/builtin_function, line 686 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We should add a test for the errArgTooBig case.

Done.

Comments from Reviewable

nvanbenschoten · 2016-08-26T15:49:50Z

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

sql/parser/builtins.go, line 1406 [r1] (raw file):

Previously, knz (kena) wrote…

I'm not comfortable using switch here because switch suggests the conditions are mutually exclusive and thus that the ordering of tests is indeterminate. Here the conditions have to be evaluated in sequence.

A switch statement still gives you that property, but ok.

sql/parser/builtins.go, line 1436 [r1] (raw file):

Previously, knz (kena) wrote…

Thanks for the tip! I'm using the Sizeof solution now.

We might want to just export this from `util/decimal` as `decimal.WordSize` and use that here and in the `encoding` package. No strong feelings.

sql/parser/builtins.go, line 1453 [r1] (raw file):

Previously, knz (kena) wrote…

Done.

Much better, thanks,

sql/parser/builtins.go, line 1420 [r2] (raw file):

  v, frac := math.Modf(x * pow)
  // The following computation implements unbiased rounding, also called
  // bankers' rounding. It ensures that when values that fall exactly

that when values that? The second sentence doesnt quite make sense to me.

Comments from Reviewable

Prior to this patch the computation would take too long or too much memory if EXP was given an argument too large or if the 2nd ROUND argument was negative with a large absolute value. This patch addresses this issue by limiting the range of acceptable arguments (1024 for EXP, anything that causes more than 2000 extra "0" digits to be appended on the right for ROUND). It improves both functions as follows: - a negative ROUND scale is now supported as in PostgreSQL and Oracle; this rounds to positions on the left of the decimal separator. - the ROUND function for floats is now performed on floats directly, instead of using Printf/ParseFloat as previously (faster). - the ROUND function for decimals is now faster if the 2nd argument is negative and its absolute value is larger than the number of digits in the 1st argument on the left of its decimal separator. - ROUND for both floats and decimal now use half to even rounding, also called unbiased rounding or bankers' rounding.

knz · 2016-08-27T11:10:18Z

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

sql/parser/builtins.go, line 1436 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We might want to just export this from util/decimal as decimal.WordSize and use that here and in the encoding package. No strong feelings.

Done.

sql/parser/builtins.go, line 1420 [r2] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

that when values that? The second sentence doesnt quite make sense to me.

Done.

Comments from Reviewable

knz assigned maddyblue and nvanbenschoten Aug 25, 2016

knz added the docs-todo label Aug 25, 2016

knz reviewed Aug 25, 2016
View reviewed changes

knz force-pushed the fix-round branch from f069ee7 to 922ceef Compare August 25, 2016 22:10

knz force-pushed the fix-round branch from 922ceef to 6655414 Compare August 27, 2016 11:10

knz merged commit 9405b1d into cockroachdb:develop Aug 27, 2016

knz deleted the fix-round branch August 27, 2016 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: improve the ROUND and EXP functions. #8822

sql: improve the ROUND and EXP functions. #8822

knz commented Aug 25, 2016 •

edited

knz Aug 25, 2016

maddyblue commented Aug 25, 2016

nvanbenschoten commented Aug 25, 2016

maddyblue commented Aug 25, 2016

knz commented Aug 25, 2016

nvanbenschoten commented Aug 26, 2016

knz commented Aug 27, 2016

sql: improve the ROUND and EXP functions. #8822

sql: improve the ROUND and EXP functions. #8822

Conversation

knz commented Aug 25, 2016 • edited

knz Aug 25, 2016

Choose a reason for hiding this comment

maddyblue commented Aug 25, 2016

nvanbenschoten commented Aug 25, 2016

maddyblue commented Aug 25, 2016

knz commented Aug 25, 2016

nvanbenschoten commented Aug 26, 2016

knz commented Aug 27, 2016

knz commented Aug 25, 2016 •

edited