Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
sql/parser: avoid float -> unsigned conversions
While investigating this, I discovered that our current `round`
implementation is quite dubious, and does not rely on any referenced
source material or any material that I could find. I also found that
Postgres does not implement 2-ary `round` where the first argument is
a float.

The above is addressed by:
- replacing `round(float)` with a transcription of Postgres' `rint`.
- replacing `round(float, int)` with an implementation that round-trips
  through apd. This is likely much slower, but likely correct.

Updates #14405.
  • Loading branch information
tamird committed May 10, 2017
1 parent 6dda974 commit b8d1c9c
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 32 deletions.
57 changes: 56 additions & 1 deletion pkg/cmd/metacheck/main.go
Expand Up @@ -17,6 +17,8 @@
package main

import (
"go/ast"
"go/types"
"log"
"os"

Expand All @@ -38,7 +40,9 @@ func (m *metaChecker) Init(program *lint.Program) {
}

func (m *metaChecker) Funcs() map[string]lint.Func {
funcs := make(map[string]lint.Func)
funcs := map[string]lint.Func{
"FloatToUnsigned": checkConvertFloatToUnsigned,
}
for _, checker := range m.checkers {
for k, v := range checker.Funcs() {
if _, ok := funcs[k]; ok {
Expand All @@ -51,6 +55,57 @@ func (m *metaChecker) Funcs() map[string]lint.Func {
return funcs
}

// @ianlancetaylor via golang-nuts[0]:
//
// For the record, the spec says, in https://golang.org/ref/spec#Conversions:
// "In all non-constant conversions involving floating-point or complex
// values, if the result type cannot represent the value the conversion
// succeeds but the result value is implementation-dependent." That is the
// case that applies here: you are converting a negative floating point number
// to uint64, which can not represent a negative value, so the result is
// implementation-dependent. The conversion to int64 works, of course. And
// the conversion to int64 and then to uint64 succeeds in converting to int64,
// and when converting to uint64 follows a different rule: "When converting
// between integer types, if the value is a signed integer, it is sign
// extended to implicit infinite precision; otherwise it is zero extended. It
// is then truncated to fit in the result type's size."
//
// So, basically, don't convert a negative floating point number to an
// unsigned integer type.
//
// [0] https://groups.google.com/d/msg/golang-nuts/LH2AO1GAIZE/PyygYRwLAwAJ
//
// TODO(tamird): upstream this.
func checkConvertFloatToUnsigned(j *lint.Job) {
fn := func(node ast.Node) bool {
call, ok := node.(*ast.CallExpr)
if !ok {
return true
}
castType, ok := j.Program.Info.TypeOf(call.Fun).(*types.Basic)
if !ok {
return true
}
if castType.Info()&types.IsUnsigned == 0 {
return true
}
for _, arg := range call.Args {
argType, ok := j.Program.Info.TypeOf(arg).(*types.Basic)
if !ok {
continue
}
if argType.Info()&types.IsFloat == 0 {
continue
}
j.Errorf(arg, "do not convert a floating point number to an unsigned integer type")
}
return true
}
for _, f := range j.Program.Files {
ast.Inspect(f, fn)
}
}

func main() {
unusedChecker := unused.NewChecker(unused.CheckAll)
unusedChecker.WholeProgram = true
Expand Down
106 changes: 80 additions & 26 deletions pkg/sql/parser/builtins.go
Expand Up @@ -1263,7 +1263,7 @@ var Builtins = map[string][]Builtin{

"round": {
floatBuiltin1(func(x float64) (Datum, error) {
return round(x, 0)
return NewDFloat(DFloat(round(x))), nil
}, "Rounds `val` to the nearest integer using half to even (banker's) rounding."),
decimalBuiltin1(func(x *apd.Decimal) (Datum, error) {
return roundDecimal(x, 0)
Expand All @@ -1273,7 +1273,25 @@ var Builtins = map[string][]Builtin{
Types: ArgTypes{{"input", TypeFloat}, {"decimal_accuracy", TypeInt}},
ReturnType: fixedReturnType(TypeFloat),
fn: func(_ *EvalContext, args Datums) (Datum, error) {
return round(float64(*args[0].(*DFloat)), int64(MustBeDInt(args[1])))
var x apd.Decimal
if _, err := x.SetFloat64(float64(*args[0].(*DFloat))); err != nil {
return nil, err
}

// TODO(mjibson): make sure this fits in an int32.
scale := int32(MustBeDInt(args[1]))

var d apd.Decimal
if _, err := RoundCtx.Quantize(&d, &x, -scale); err != nil {
return nil, err
}

f, err := d.Float64()
if err != nil {
return nil, err
}

return NewDFloat(DFloat(f)), nil
},
Info: "Keeps `decimal_accuracy` number of figures to the right of the zero position " +
" in `input` using half to even (banker's) rounding.",
Expand Down Expand Up @@ -2082,36 +2100,72 @@ func overlay(s, to string, pos, size int) (Datum, error) {
return NewDString(string(runes[:pos]) + to + string(runes[after:])), nil
}

func round(x float64, n int64) (Datum, error) {
pow := math.Pow(10, float64(n))
// Transcribed from Postgres' src/port/rint.c, with c-style comments preserved
// for ease of mapping.
//
// https://github.com/postgres/postgres/blob/REL9_6_3/src/port/rint.c
func round(x float64) float64 {
/* Per POSIX, NaNs must be returned unchanged. */
if math.IsNaN(x) {
return x
}

if pow == 0 {
// Rounding to so many digits on the left that we're underflowing.
// Avoid a NaN below.
return NewDFloat(DFloat(0)), nil
/* Both positive and negative zero should be returned unchanged. */
if x == 0.0 {
return x
}
if math.Abs(x*pow) > 1e17 {
// Rounding touches decimals below float precision; the operation
// is a no-op.
return NewDFloat(DFloat(x)), nil

roundFn := math.Ceil
if math.Signbit(x) {
roundFn = math.Floor
}

v, frac := math.Modf(x * pow)
// The following computation implements unbiased rounding, also
// called bankers' rounding. It ensures that values that fall
// exactly between two integers get equal chance to be rounded up or
// down.
if x > 0.0 {
if frac > 0.5 || (frac == 0.5 && uint64(v)%2 != 0) {
v += 1.0
}
} else {
if frac < -0.5 || (frac == -0.5 && uint64(v)%2 != 0) {
v -= 1.0
}
/*
* Subtracting 0.5 from a number very close to -0.5 can round to
* exactly -1.0, producing incorrect results, so we take the opposite
* approach: add 0.5 to the negative number, so that it goes closer to
* zero (or at most to +0.5, which is dealt with next), avoiding the
* precision issue.
*/
xOrig := x
x -= math.Copysign(0.5, x)

/*
* Be careful to return minus zero when input+0.5 >= 0, as that's what
* rint() should return with negative input.
*/
if x == 0 || math.Signbit(x) != math.Signbit(xOrig) {
return math.Copysign(0.0, xOrig)
}

/*
* For very big numbers the input may have no decimals. That case is
* detected by testing x+0.5 == x+1.0; if that happens, the input is
* returned unchanged. This also covers the case of minus infinity.
*/
if x == xOrig-math.Copysign(1.0, x) {
return xOrig
}

/* Otherwise produce a rounded estimate. */
r := roundFn(x)

/*
* If the rounding did not produce exactly input+0.5 then we're done.
*/
if r != x {
return r
}

return NewDFloat(DFloat(v / pow)), nil
/*
* The original fractional part was exactly 0.5 (since
* floor(input+0.5) == input+0.5). We need to round to nearest even.
* Dividing input+0.5 by 2, taking the floor and multiplying by 2
* yields the closest even number. This part assumes that division by
* 2 is exact, which should be OK because underflow is impossible
* here: x is an integer.
*/
return roundFn(x*0.5) * 2.0
}

func roundDecimal(x *apd.Decimal, n int32) (Datum, error) {
Expand Down
7 changes: 7 additions & 0 deletions pkg/sql/parser/decimal.go
Expand Up @@ -32,4 +32,11 @@ var (
ExactCtx = DecimalCtx.WithPrecision(0)
// HighPrecisionCtx is a decimal context with high precision.
HighPrecisionCtx = DecimalCtx.WithPrecision(2000)
// RoundCtx is a decimal context with high precision and RoundHalfEven
// rounding.
RoundCtx = func() *apd.Context {
ctx := *HighPrecisionCtx
ctx.Rounding = apd.RoundHalfEven
return &ctx
}()
)
13 changes: 8 additions & 5 deletions pkg/sql/testdata/logic_test/builtin_function
Expand Up @@ -655,6 +655,9 @@ SELECT radians(-45.0), radians(45.0)
----
-0.7853981633974483 0.7853981633974483

query error invalid operation
SELECT round(123.456::float, -2438602134409251682)

query RRR
SELECT round(4.2::float, 0), round(4.2::float, 10), round(4.22222222::decimal, 3)
----
Expand Down Expand Up @@ -710,12 +713,12 @@ SELECT round(-1.7976931348623157e+308::float, 1), round(1.7976931348623157e+308:
query RR
SELECT round(-1.7976931348623157e+308::float, -303), round(1.7976931348623157e+308::float, -303)
----
-1.797690000000001e+308 1.797690000000001e+308
-1.79769e+308 1.79769e+308

query RR
SELECT round(-1.23456789e+308::float, -308), round(1.23456789e+308::float, -308)
----
-1.0000000000000006e+308 1.0000000000000006e+308
-1e+308 1e+308

query RR
SELECT round(-1.7976931348623157e-308::float, 1), round(1.7976931348623157e-308::float, 1)
Expand All @@ -727,10 +730,10 @@ SELECT 1.234567890123456789::float, round(1.234567890123456789::float,15), roun
----
1.2345678901234567 1.234567890123457 1.2345678901234567 1.2345678901234567

query RRRR
SELECT round(123.456::float, -1), round(123.456::float, -2), round(123.456::float, -3), round(123.456::float, -2438602134409251682)
query RRR
SELECT round(123.456::float, -1), round(123.456::float, -2), round(123.456::float, -3)
----
120 100 0 0
120 100 0

query RRRR
SELECT round(123.456::decimal, -1), round(123.456::decimal, -2), round(123.456::decimal, -3), round(123.456::decimal, -200)
Expand Down

0 comments on commit b8d1c9c

Please sign in to comment.