From ac09c11badb630a375d65b930e606f17e3e14662 Mon Sep 17 00:00:00 2001 From: Nia Weiss Date: Tue, 26 Jan 2021 12:30:13 -0500 Subject: [PATCH 1/2] Invoke precalls directly in count operations This changes Count(Precall()) operations to execute the precall directly inside of the count operation, bypassing the transformation to a Precomputed() call. Eliminating the Precomputed() step causes Count(Distinct()) to work properly on negative integers. --- executor.go | 52 ++++++++++++++++++++++++++---------------------- executor_test.go | 24 ++++++++++++++++++++++ pql/ast.go | 9 ++++++--- 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/executor.go b/executor.go index b1a0ae1eb..7e046c11c 100644 --- a/executor.go +++ b/executor.go @@ -556,11 +556,22 @@ func (e *executor) execute(ctx context.Context, qcx *Qcx, index string, q *pql.Q // about the positive values, because only positive values // are valid column IDs. So we don't actually eat top-level // pre calls. - err := e.handlePreCallChildren(ctx, qcx, index, call, shards, opt) - if err != nil { - return nil, err + if call.Name == "Count" { + // Handle count specially, skipping the level directly underneath it. + for _, child := range call.Children { + err := e.handlePreCallChildren(ctx, qcx, index, child, shards, opt) + if err != nil { + return nil, err + } + } + } else { + err := e.handlePreCallChildren(ctx, qcx, index, call, shards, opt) + if err != nil { + return nil, err + } } var v interface{} + var err error // Top-level calls don't need to precompute cross-index things, // because we can just pick whatever index we want, but we // still need to handle them. Since everything else was @@ -4618,28 +4629,21 @@ func (e *executor) executeCount(ctx context.Context, qcx *Qcx, index string, c * child := c.Children[0] - // If the child is precomputed, we'll bypass mapreduce, ignore - // shards, and just count the number of bits. - if child.Name == "Precomputed" { - count := uint64(0) - for _, irow := range child.Precomputed { - switch row := irow.(type) { - case *Row: - for _, seg := range row.segments { - count += seg.n - } - case SignedRow: - for _, seg := range row.Pos.segments { - count += seg.n - } - for _, seg := range row.Neg.segments { - count += seg.n - } - default: - return 0, errors.Errorf("unexpected precomputed value type inside count: %+v", row) - } + // If the child is distinct/similar, execute it directly here and count the result. + if child.Type == pql.PrecallGlobal { + result, err := e.executeCall(ctx, qcx, index, child, shards, opt) + if err != nil { + return 0, err + } + + switch row := result.(type) { + case *Row: + return row.Count(), nil + case SignedRow: + return row.Pos.Count() + row.Neg.Count(), nil + default: + return 0, errors.Errorf("cannot count result of type %T from call %q", row, child.String()) } - return count, nil } // Execute calls in bulk on each remote node and merge. diff --git a/executor_test.go b/executor_test.go index 6a202ca3a..ae86ba7d6 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6569,6 +6569,30 @@ func TestExecutor_Execute_CountDistinct(t *testing.T) { }) } +// Ensure that Count(Distinct()) works with negative numbers. +func TestExecutor_Execute_CountDistinctSigned(t *testing.T) { + c := test.MustRunCluster(t, 2) + defer c.Close() + + c.CreateField(t, "i", pilosa.IndexOptions{}, "ints", + pilosa.OptFieldTypeInt(-100, 100), + ) + + c.Query(t, "i", fmt.Sprintf(` + Set(0, ints=1) + Set(%d, ints=-1) + `, ShardWidth)) + + resp := c.Query(t, "i", "Count(Distinct(field=ints))") + cnt, ok := resp.Results[0].(uint64) + if !ok { + t.Fatalf("invalid response type, expected: uint64, got: %T", resp.Results[0]) + } + if cnt != 2 { + t.Fatalf("invalid result, expected: 2, got: %v", cnt) + } +} + // Ensure that a top-level, bare distinct on multiple nodes // is handled correctly. func TestExecutor_BareDistinct(t *testing.T) { diff --git a/pql/ast.go b/pql/ast.go index 54ae2dee7..a4b80f4ae 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -274,14 +274,17 @@ type callStackElem struct { type CallType byte const ( - // Normal calls can be executed per shard. + // PrecallNone calls can be executed per shard. PrecallNone = CallType(iota) - // PreCallGlobal indicates a call which must be run globally *before* + + // PrecallGlobal indicates a call which must be run globally *before* // distributing the call to other shards. Example: A Distinct query, // where every shard could potentially produce results for any shard, // so you have to produce the results up front. + // These are processed directly when inside of a count operation. PrecallGlobal - // PreCallPerNode indicates a call which needs to be run per-shard + + // PrecallPerNode indicates a call which needs to be run per-shard // in a way that lets it be done on each shard, but where it should // be done prior to spawning per-shard goroutines. Example: // A cross-index query, where each local shard may or may not need From 0d97ec559b5f2b62188dfaee955e49d9400cf2dd Mon Sep 17 00:00:00 2001 From: Nia Weiss Date: Wed, 27 Jan 2021 10:29:40 -0500 Subject: [PATCH 2/2] switch signed count distinct test to use TestVariousQueries --- executor_test.go | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/executor_test.go b/executor_test.go index ae86ba7d6..47e1e6d57 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6569,30 +6569,6 @@ func TestExecutor_Execute_CountDistinct(t *testing.T) { }) } -// Ensure that Count(Distinct()) works with negative numbers. -func TestExecutor_Execute_CountDistinctSigned(t *testing.T) { - c := test.MustRunCluster(t, 2) - defer c.Close() - - c.CreateField(t, "i", pilosa.IndexOptions{}, "ints", - pilosa.OptFieldTypeInt(-100, 100), - ) - - c.Query(t, "i", fmt.Sprintf(` - Set(0, ints=1) - Set(%d, ints=-1) - `, ShardWidth)) - - resp := c.Query(t, "i", "Count(Distinct(field=ints))") - cnt, ok := resp.Results[0].(uint64) - if !ok { - t.Fatalf("invalid response type, expected: uint64, got: %T", resp.Results[0]) - } - if cnt != 2 { - t.Fatalf("invalid result, expected: 2, got: %v", cnt) - } -} - // Ensure that a top-level, bare distinct on multiple nodes // is handled correctly. func TestExecutor_BareDistinct(t *testing.T) { @@ -6842,6 +6818,8 @@ func TestMissingKeyRegression(t *testing.T) { func TestVariousQueries(t *testing.T) { for _, clusterSize := range []int{1, 3, 4, 7} { t.Run(fmt.Sprintf("%d-node", clusterSize), func(t *testing.T) { + t.Parallel() + variousQueries(t, clusterSize) }) } @@ -6921,8 +6899,7 @@ func variousQueries(t *testing.T, clusterSize int) { {Val: 0, Key: "userE"}, }) - // Create and populate "affinity" int field with negative, positive, zero and null values. - + // Create and populate "net_worth" int field with positive values. c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "net_worth", pilosa.OptFieldTypeInt(-100000000, 100000000)) c.ImportIntKey(t, "users", "net_worth", []test.IntKey{ {Val: 1, Key: "userA"}, @@ -7056,6 +7033,15 @@ toronto,2,11 }, csvVerifier: "-10\n-5\n0\n5\n10\n", }, + { + query: "Count(Distinct(field=affinity))", + qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) { + if resp.Results[0].(uint64) != 5 { + t.Errorf("wrong number of values: %+v", resp.Results[0]) + } + }, + csvVerifier: "5\n", + }, { query: "Distinct(Row(affinity>=0),field=affinity)", qrVerifier: func(t *testing.T, resp pilosa.QueryResponse) {