also fix Sum query, but don't convert to float until the last step

this avoids compounding floating point errors while summing up the
numbers, and means less logic needs to change. Should probably convert
min and max to use this approach as well, though they don't suffer
from the compounding error issue, it is simpler.
This commit is contained in:
Matt Jaffee 2020-02-21 13:51:32 -06:00
parent 7321f9427c
commit fe46c84d19
No known key found for this signature in database
GPG key ID: 08A3DFFF987B11BF
2 changed files with 59 additions and 1 deletions

View file

@ -770,7 +770,8 @@ func (e *executor) executeSum(ctx context.Context, index string, c *pql.Call, sh
span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeSum")
defer span.Finish()
if field := c.Args["field"]; field == "" {
fieldName, ok := c.Args["field"].(string)
if !ok || fieldName == "" {
return ValCount{}, errors.New("Sum(): field required")
}
@ -798,6 +799,21 @@ func (e *executor) executeSum(ctx context.Context, index string, c *pql.Call, sh
if other.Count == 0 {
return ValCount{}, nil
}
// scale summed response into float if decimal field and this is
// not a remote query (we're about to return to original client).
if !opt.Remote {
field := e.Holder.Field(index, fieldName)
if field == nil {
return ValCount{}, ErrFieldNotFound
}
if field.Type() == FieldTypeDecimal {
if scale := field.Options().Scale; scale != 0 {
other.FloatVal = float64(other.Val) / math.Pow10(int(scale))
other.Val = 0
}
}
}
return other, nil
}

View file

@ -1019,3 +1019,45 @@ func TestClusterExhaustingConnectionsImport(t *testing.T) {
t.Fatalf("setting lots of shards: %v", err)
}
}
func TestClusterMinMaxSumDecimal(t *testing.T) {
cluster := test.MustRunCluster(t, 3)
defer cluster.Close()
cmd := cluster[0]
cmd.MustCreateIndex(t, "testdec", pilosa.IndexOptions{Keys: true, TrackExistence: true})
cmd.MustCreateField(t, "testdec", "adec", pilosa.OptFieldTypeDecimal(2))
test.MustDo("POST", cluster[0].URL()+"/index/testdec/query", `
Set("a", adec=42.2)
Set("b", adec=11.12)
Set("c", adec=13.41)
Set("d", adec=99.87)
Set("e", adec=11.13)
Set("f", adec=12.12)
Set("g", adec=15.52)
Set("h", adec=100.22)
`)
result := test.MustDo("POST", cluster[0].URL()+"/index/testdec/query", "Sum(field=adec)")
if !strings.Contains(result.Body, `"floatValue":305.59`) {
t.Fatalf("expected float sum of 305.59, but got: '%s'", result.Body)
} else if !strings.Contains(result.Body, `"count":8`) {
t.Fatalf("expected count 8, but got: '%s'", result.Body)
}
result = test.MustDo("POST", cluster[0].URL()+"/index/testdec/query", "Max(field=adec)")
if !strings.Contains(result.Body, `"floatValue":100.22`) {
t.Fatalf("expected float max of 100.22, but got: '%s'", result.Body)
} else if !strings.Contains(result.Body, `"count":1`) {
t.Fatalf("expected count 1, but got: '%s'", result.Body)
}
result = test.MustDo("POST", cluster[0].URL()+"/index/testdec/query", "Min(field=adec)")
if !strings.Contains(result.Body, `"floatValue":11.12`) {
t.Fatalf("expected float min of 11.12, but got: '%s'", result.Body)
} else if !strings.Contains(result.Body, `"count":1`) {
t.Fatalf("expected count 1, but got: '%s'", result.Body)
}
}