From 40e3acc3e4e89a9a4ba427aa1c2598eb64f92ce5 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Tue, 23 Mar 2021 16:57:37 -0500 Subject: [PATCH 01/12] change percentile value ranges from 0-1 to 0-100 --- executor.go | 4 ++-- executor_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/executor.go b/executor.go index 9ee05633e..a97d74871 100644 --- a/executor.go +++ b/executor.go @@ -1306,8 +1306,8 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string var nth float64 if nthArg, ok := c.Args["nth"].(pql.Decimal); ok { nth = nthArg.Float64() - if nth < 0 || nth > 1.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value(%f), should be >= 0 and <= 1.0", nth) + if nth < 0 || nth > 100.0 { + return ValCount{}, errors.Errorf("Percentile(): invalid nth value(%f), should be >= 0 and <= 100", nth) } } else { return ValCount{}, errors.New("Percentile(): nth required") diff --git a/executor_test.go b/executor_test.go index 3f33c8415..2f07b82f9 100644 --- a/executor_test.go +++ b/executor_test.go @@ -7053,7 +7053,7 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { if nth == 0.0 { return min } - k := (1 - nth) / nth + k := (100 - nth) / nth possibleNthVal := int64(0) // bin search @@ -7127,7 +7127,7 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { } // generate test cases per each nth argument - nths := []float64{0.0, 0.1, 0.25, 0.5, 0.75, 0.9, 0.99} + nths := []float64{0.0, 10, 25, 50, 75, 90, 99.99} var tests []testCase for _, nth := range nths { query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%f)`, nth) From d3438e8a805693111abf06d46cc72b695d9bb835 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Wed, 24 Mar 2021 10:58:47 -0500 Subject: [PATCH 02/12] correct k calculation and change test nth values --- executor.go | 2 +- executor_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index a97d74871..0302c0c9b 100644 --- a/executor.go +++ b/executor.go @@ -1365,7 +1365,7 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string rangeCall = intersectCall.Children[0] } - k := (1 - nth) / nth + k := (100 - nth) / nth min, max := minVal.Val, maxVal.Val // estimate nth val, eg median when nth=0.5 diff --git a/executor_test.go b/executor_test.go index 2f07b82f9..40e606c35 100644 --- a/executor_test.go +++ b/executor_test.go @@ -7127,7 +7127,7 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { } // generate test cases per each nth argument - nths := []float64{0.0, 10, 25, 50, 75, 90, 99.99} + nths := []float64{0.0, 10, 25, 50, 75, 90, 99} var tests []testCase for _, nth := range nths { query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%f)`, nth) From 61beef7e5ad05d57ec191777e45bd53fb4e76ea8 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Thu, 25 Mar 2021 16:27:55 -0500 Subject: [PATCH 03/12] add Int64() function and include case of int64 --- executor.go | 17 +++++++++++------ pql/decimal.go | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/executor.go b/executor.go index 0302c0c9b..e4e41abbb 100644 --- a/executor.go +++ b/executor.go @@ -1303,11 +1303,16 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string defer span.Finish() // get nth - var nth float64 + var nthFloat float64 if nthArg, ok := c.Args["nth"].(pql.Decimal); ok { - nth = nthArg.Float64() - if nth < 0 || nth > 100.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value(%f), should be >= 0 and <= 100", nth) + switch c.Args["nth"].(type) { + case pql.Decimal: + nthFloat = nthArg.Float64() + case int64: + nthFloat = float64(nthArg.Int64()) + } + if nthFloat < 0 || nthFloat > 100.0 { + return ValCount{}, errors.Errorf("Percentile(): invalid nth value(%f), should be >= 0 and <= 100", nthFloat) } } else { return ValCount{}, errors.New("Percentile(): nth required") @@ -1337,7 +1342,7 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string if err != nil { return ValCount{}, errors.Wrap(err, "executing Min call for Percentile") } - if nth == 0.0 { + if nthFloat == 0.0 { return ValCount{Val: minVal.Val, Count: minVal.Count}, nil } @@ -1365,7 +1370,7 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string rangeCall = intersectCall.Children[0] } - k := (100 - nth) / nth + k := (100 - nthFloat) / nthFloat min, max := minVal.Val, maxVal.Val // estimate nth val, eg median when nth=0.5 diff --git a/pql/decimal.go b/pql/decimal.go index 61843f580..b9d1e3749 100644 --- a/pql/decimal.go +++ b/pql/decimal.go @@ -245,6 +245,20 @@ func (d Decimal) Float64() float64 { return ret } +// Int64 returns d as a int64. +// TODO: this could very easily lose precision; we should audit +// its use and protect against unexpected results. +func (d Decimal) Int64() int64 { + var ret int64 + if d.Scale == 0 { + ret = int64(d.Value) + } else { + temp := float64(d.Value) / math.Pow10(int(d.Scale)) + ret = int64(temp) + } + return ret +} + // String returns the string representation of the decimal. func (d Decimal) String() string { var s string From 8746444cdbfc037a37b7f051fbe4ae39213c7b31 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Thu, 25 Mar 2021 16:57:08 -0500 Subject: [PATCH 04/12] add error message for invalid type --- executor.go | 7 +++++-- pql/decimal.go | 14 -------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/executor.go b/executor.go index e4e41abbb..832ce3f0f 100644 --- a/executor.go +++ b/executor.go @@ -20,6 +20,7 @@ import ( "fmt" "math" "math/bits" + "reflect" "sort" "strings" "sync" @@ -1309,10 +1310,12 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string case pql.Decimal: nthFloat = nthArg.Float64() case int64: - nthFloat = float64(nthArg.Int64()) + nthFloat = float64(nthArg.ToInt64(0)) + default: + return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%f), should be int64 or pql.Decimal", reflect.TypeOf(nthArg)) } if nthFloat < 0 || nthFloat > 100.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value(%f), should be >= 0 and <= 100", nthFloat) + return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be >= 0 and <= 100", nthFloat) } } else { return ValCount{}, errors.New("Percentile(): nth required") diff --git a/pql/decimal.go b/pql/decimal.go index b9d1e3749..61843f580 100644 --- a/pql/decimal.go +++ b/pql/decimal.go @@ -245,20 +245,6 @@ func (d Decimal) Float64() float64 { return ret } -// Int64 returns d as a int64. -// TODO: this could very easily lose precision; we should audit -// its use and protect against unexpected results. -func (d Decimal) Int64() int64 { - var ret int64 - if d.Scale == 0 { - ret = int64(d.Value) - } else { - temp := float64(d.Value) / math.Pow10(int(d.Scale)) - ret = int64(temp) - } - return ret -} - // String returns the string representation of the decimal. func (d Decimal) String() string { var s string From 87412068f1a22db5709752d0a569ac1820125509 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Fri, 26 Mar 2021 13:01:31 -0500 Subject: [PATCH 05/12] add new test to account for int64 nth values --- executor_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/executor_test.go b/executor_test.go index 40e606c35..e5e5a809f 100644 --- a/executor_test.go +++ b/executor_test.go @@ -7127,9 +7127,9 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { } // generate test cases per each nth argument - nths := []float64{0.0, 10, 25, 50, 75, 90, 99} + nthsFloat := []float64{0, 10, 25, 50, 75, 90, 99} var tests []testCase - for _, nth := range nths { + for _, nth := range nthsFloat { query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%f)`, nth) expectedPercentile := getExpectedPercentile(nums, nth) tests = append(tests, testCase{ @@ -7137,6 +7137,15 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { csvVerifier: fmt.Sprintf("%d,1\n", expectedPercentile), }) } + nthsInt := []int64{0, 10, 25, 50, 75, 90, 99} + for _, nth := range nthsInt { + query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%d)`, nth) + expectedPercentile := getExpectedPercentile(nums, float64(nth)) + tests = append(tests, testCase{ + query: query, + csvVerifier: fmt.Sprintf("%d,1\n", expectedPercentile), + }) + } for i, tst := range tests { t.Run(fmt.Sprintf("%d-%s", i, tst.query), func(t *testing.T) { From 6c94bfdb5ecf84b77aa2cb04f5c750ac963740b7 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Fri, 26 Mar 2021 13:56:22 -0500 Subject: [PATCH 06/12] improve error responses and change switch statement to allow int64 --- executor.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/executor.go b/executor.go index 832ce3f0f..cfc5acfff 100644 --- a/executor.go +++ b/executor.go @@ -1305,19 +1305,24 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string // get nth var nthFloat float64 - if nthArg, ok := c.Args["nth"].(pql.Decimal); ok { - switch c.Args["nth"].(type) { - case pql.Decimal: - nthFloat = nthArg.Float64() - case int64: - nthFloat = float64(nthArg.ToInt64(0)) - default: - return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%f), should be int64 or pql.Decimal", reflect.TypeOf(nthArg)) - } + OK := false + switch c.Args["nth"].(type) { + case pql.Decimal: + nthArg, ok := c.Args["nth"].(pql.Decimal) + nthFloat = nthArg.Float64() + OK = ok + case int64: + nthArg, ok := c.Args["nth"].(int64) + nthFloat = float64(nthArg) + OK = ok + default: + return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%f), should be int64 or pql.Decimal", reflect.TypeOf(c.Args["nth"])) + } + if OK { if nthFloat < 0 || nthFloat > 100.0 { return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be >= 0 and <= 100", nthFloat) } - } else { + } else if c.Args["nth"] == nil { return ValCount{}, errors.New("Percentile(): nth required") } From f06c13abcdba525a87f8d0dc39240a8d91a1aadd Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Fri, 26 Mar 2021 16:11:16 -0500 Subject: [PATCH 07/12] alter incorrect type error message --- executor.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/executor.go b/executor.go index cfc5acfff..7b95cda29 100644 --- a/executor.go +++ b/executor.go @@ -20,7 +20,6 @@ import ( "fmt" "math" "math/bits" - "reflect" "sort" "strings" "sync" @@ -1316,7 +1315,7 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string nthFloat = float64(nthArg) OK = ok default: - return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%f), should be int64 or pql.Decimal", reflect.TypeOf(c.Args["nth"])) + return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%T), should be int64 or pql.Decimal", c.Args["nth"]) } if OK { if nthFloat < 0 || nthFloat > 100.0 { From 4ce71eeddf64f21a061cfa8e878723dbfe19671b Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Fri, 26 Mar 2021 16:54:44 -0500 Subject: [PATCH 08/12] change nth value limit message and number of test int64 nth values --- executor.go | 2 +- executor_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index 7b95cda29..39aea216d 100644 --- a/executor.go +++ b/executor.go @@ -1319,7 +1319,7 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string } if OK { if nthFloat < 0 || nthFloat > 100.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be >= 0 and <= 100", nthFloat) + return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be between 0 and 100 inclusive", nthFloat) } } else if c.Args["nth"] == nil { return ValCount{}, errors.New("Percentile(): nth required") diff --git a/executor_test.go b/executor_test.go index e5e5a809f..62a2784b1 100644 --- a/executor_test.go +++ b/executor_test.go @@ -7137,7 +7137,7 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { csvVerifier: fmt.Sprintf("%d,1\n", expectedPercentile), }) } - nthsInt := []int64{0, 10, 25, 50, 75, 90, 99} + nthsInt := []int64{10} for _, nth := range nthsInt { query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%d)`, nth) expectedPercentile := getExpectedPercentile(nums, float64(nth)) From 377c3f36544d3acc9bea33680fd562a9ffdaf199 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Mon, 29 Mar 2021 15:59:30 -0500 Subject: [PATCH 09/12] rework checks and reword incorrect type error message --- executor.go | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/executor.go b/executor.go index 39aea216d..a5a824559 100644 --- a/executor.go +++ b/executor.go @@ -1304,26 +1304,21 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string // get nth var nthFloat float64 - OK := false - switch c.Args["nth"].(type) { - case pql.Decimal: - nthArg, ok := c.Args["nth"].(pql.Decimal) - nthFloat = nthArg.Float64() - OK = ok - case int64: - nthArg, ok := c.Args["nth"].(int64) - nthFloat = float64(nthArg) - OK = ok - default: - return ValCount{}, errors.Errorf("Percentile(): invalid nth type (%T), should be int64 or pql.Decimal", c.Args["nth"]) - } - if OK { - if nthFloat < 0 || nthFloat > 100.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be between 0 and 100 inclusive", nthFloat) - } - } else if c.Args["nth"] == nil { + nthArg := c.Args["nth"] + if nthArg == nil { return ValCount{}, errors.New("Percentile(): nth required") } + switch nthArg.(type) { + case pql.Decimal: + nthFloat = nthArg.(pql.Decimal).Float64() + case int64: + nthFloat = float64(nthArg.(int64)) + default: + return ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be int64 or pql.Decimal", c.Args["nth"]) + } + if nthFloat < 0 || nthFloat > 100.0 { + return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be between 0 and 100 inclusive", nthFloat) + } // get field if fieldArg := c.Args["field"]; fieldArg == "" { From 4c8dc8d54f4485ac29e577c975f43a43c193f4b3 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Mon, 29 Mar 2021 16:24:56 -0500 Subject: [PATCH 10/12] address variable declaration warning --- executor.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/executor.go b/executor.go index a5a824559..65fe1b0c7 100644 --- a/executor.go +++ b/executor.go @@ -1308,11 +1308,11 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string if nthArg == nil { return ValCount{}, errors.New("Percentile(): nth required") } - switch nthArg.(type) { + switch nthArg := nthArg.(type) { case pql.Decimal: - nthFloat = nthArg.(pql.Decimal).Float64() + nthFloat = nthArg.Float64() case int64: - nthFloat = float64(nthArg.(int64)) + nthFloat = float64(nthArg) default: return ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be int64 or pql.Decimal", c.Args["nth"]) } From 13ec97abf52761e60c65ba0b3dab30b608513805 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Mon, 29 Mar 2021 16:55:50 -0500 Subject: [PATCH 11/12] add new test cases for limits and clarify error messages --- executor.go | 9 ++++++--- executor_test.go | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/executor.go b/executor.go index 65fe1b0c7..8a0215c0b 100644 --- a/executor.go +++ b/executor.go @@ -1304,7 +1304,10 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string // get nth var nthFloat float64 - nthArg := c.Args["nth"] + nthArg, ok := c.Args["nth"] + if !ok { + return ValCount{}, errors.New("Percentile(): nth required") + } if nthArg == nil { return ValCount{}, errors.New("Percentile(): nth required") } @@ -1314,10 +1317,10 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string case int64: nthFloat = float64(nthArg) default: - return ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be int64 or pql.Decimal", c.Args["nth"]) + return ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be a number between 0 and 100 inclusive", c.Args["nth"]) } if nthFloat < 0 || nthFloat > 100.0 { - return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be between 0 and 100 inclusive", nthFloat) + return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be a number between 0 and 100 inclusive", nthFloat) } // get field diff --git a/executor_test.go b/executor_test.go index 62a2784b1..1d69f5662 100644 --- a/executor_test.go +++ b/executor_test.go @@ -7137,7 +7137,7 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) { csvVerifier: fmt.Sprintf("%d,1\n", expectedPercentile), }) } - nthsInt := []int64{10} + nthsInt := []int64{0, 10, 100} for _, nth := range nthsInt { query := fmt.Sprintf(`Percentile(field="net_worth", filter=Row(val="foo"), nth=%d)`, nth) expectedPercentile := getExpectedPercentile(nums, float64(nth)) From 8c20d9a51c333ddbf0f08ddcba06a33a50b41dc9 Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Tue, 30 Mar 2021 08:13:36 -0500 Subject: [PATCH 12/12] remove redundant error message --- executor.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/executor.go b/executor.go index 8a0215c0b..0a139369b 100644 --- a/executor.go +++ b/executor.go @@ -1308,9 +1308,6 @@ func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string if !ok { return ValCount{}, errors.New("Percentile(): nth required") } - if nthArg == nil { - return ValCount{}, errors.New("Percentile(): nth required") - } switch nthArg := nthArg.(type) { case pql.Decimal: nthFloat = nthArg.Float64()