From fe46c84d1992e652e8342fc9763a9eb9434e2ac7 Mon Sep 17 00:00:00 2001
From: Matt Jaffee <matthew.jaffee@gmail.com>
Date: Fri, 21 Feb 2020 13:51:32 -0600
Subject: [PATCH] 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.
---
 executor.go           | 18 +++++++++++++++++-
 server/server_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/executor.go b/executor.go
index 73f7e19d2..ce0707be0 100644
--- a/executor.go
+++ b/executor.go
@@ -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
 }
 
diff --git a/server/server_test.go b/server/server_test.go
index 457ab7451..5225e7a05 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -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)
+	}
+
+}