From 1372bafe026d64ad56c446e65e0ffb95e46af227 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Wed, 23 Dec 2020 18:53:17 -0600 Subject: [PATCH] fix bugs where row index and field weren't always being propagated I used a "paranoia" check to find these, but then realized the check had a ton of false positives and doing it properly wasn't going to be straightforward. I'm leaving the paranoia stuff in unless there are objections, because I've wanted it before and not had it. I also removed a log line that is very verbose and I don't think helps anyone. --- Makefile | 2 +- executor.go | 10 ++++++++-- executor_test.go | 40 ++++++++++++++++++++++++++++++++++++---- nop_paranoia.go | 19 +++++++++++++++++++ paranoia.go | 19 +++++++++++++++++++ row.go | 7 +++++++ server.go | 1 - 7 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 nop_paranoia.go create mode 100644 paranoia.go diff --git a/Makefile b/Makefile index abd07304d..d622a235c 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ RELEASE ?= 0 RELEASE_ENABLED = $(subst 0,,$(RELEASE)) BUILD_TAGS += $(if $(RELEASE_ENABLED),release) BUILD_TAGS += shardwidth$(SHARD_WIDTH) -TEST_TAGS = roaringparanoia +TEST_TAGS = roaringparanoia paranoia define LICENSE_HASH_CODE head -13 $1 | sed -e 's/Copyright 20[0-9][0-9]/Copyright 20XX/g' | shasum | cut -f 1 -d " " endef diff --git a/executor.go b/executor.go index 39bfa30f8..6694deeb1 100644 --- a/executor.go +++ b/executor.go @@ -1421,7 +1421,10 @@ func (e *executor) executeDistinctShard(ctx context.Context, qcx *Qcx, index str } bsig := field.bsiGroup(fieldName) if bsig == nil { - result = new(Row) + result = &Row{ + Index: index, + Field: fieldName, + } } else { result = SignedRow{} } @@ -5188,7 +5191,10 @@ func makeEmbeddedDataForShards(allRows []*Row, shards []uint64) []*Row { } segments := row.segments segmentIndex := 0 - newRows[i] = &Row{} + newRows[i] = &Row{ + Index: row.Index, + Field: row.Field, + } for _, shard := range shards { for segmentIndex < len(segments) && segments[segmentIndex].shard < shard { segmentIndex++ diff --git a/executor_test.go b/executor_test.go index 76174d4b6..e065004ef 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6844,7 +6844,7 @@ func TestVariousQueries(t *testing.T) { }, }, { - query: "Distinct(Row(affinity>=0), field=affinity)", + query: "Distinct(Row(affinity>=0),field=affinity)", verifier: func(t *testing.T, resp pilosa.QueryResponse) { if !reflect.DeepEqual(resp.Results[0].(pilosa.SignedRow).Pos.Columns(), []uint64{0, 5, 10}) { t.Errorf("wrong positive records: %+v", resp.Results[0].(pilosa.SignedRow).Pos.Columns()) @@ -6855,7 +6855,7 @@ func TestVariousQueries(t *testing.T) { }, }, { - query: "Count(Distinct(Row(affinity>=0), field=affinity))", + query: "Count(Distinct(Row(affinity>=0),field=affinity))", verifier: func(t *testing.T, resp pilosa.QueryResponse) { if resp.Results[0].(uint64) != 3 { t.Errorf("wrong number of values: %+v", resp.Results[0]) @@ -6877,6 +6877,30 @@ func TestVariousQueries(t *testing.T) { // } // }, // }, + { + query: "Distinct(Row(affinity<0),field=likes)", + verifier: func(t *testing.T, resp pilosa.QueryResponse) { + if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"pilosa", "zebra", "icecream"}) { + t.Errorf("wrong values: %+v", resp.Results[0]) + } + }, + }, + { + query: "Distinct(Row(affinity>0),field=likes)", + verifier: func(t *testing.T, resp pilosa.QueryResponse) { + if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pangolin", "icecream"}) { + t.Errorf("wrong values: %+v", resp.Results[0]) + } + }, + }, + { + query: "Distinct(Row(likenums=1),field=likes)", + verifier: func(t *testing.T, resp pilosa.QueryResponse) { + if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "icecream"}) { + t.Errorf("wrong values: %+v", resp.Results[0]) + } + }, + }, { query: "Distinct(field=likes)", verifier: func(t *testing.T, resp pilosa.QueryResponse) { @@ -6886,9 +6910,17 @@ func TestVariousQueries(t *testing.T) { }, }, { - query: "Distinct(Row(affinity<0), field=likes)", + query: "Distinct(All(),field=likes)", verifier: func(t *testing.T, resp pilosa.QueryResponse) { - if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"pilosa", "zebra", "icecream"}) { + if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pilosa", "pangolin", "zebra", "toucan", "dog", "icecream"}) { + t.Errorf("wrong values: %+v", resp.Results[0]) + } + }, + }, + { + query: "Distinct(field=likes )", + verifier: func(t *testing.T, resp pilosa.QueryResponse) { + if !reflect.DeepEqual(resp.Results[0].(*pilosa.Row).Keys, []string{"molecula", "pilosa", "pangolin", "zebra", "toucan", "dog", "icecream"}) { t.Errorf("wrong values: %+v", resp.Results[0]) } }, diff --git a/nop_paranoia.go b/nop_paranoia.go new file mode 100644 index 000000000..77cdae411 --- /dev/null +++ b/nop_paranoia.go @@ -0,0 +1,19 @@ +// Copyright 2020 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build !paranoia + +package pilosa + +const paranoia = false diff --git a/paranoia.go b/paranoia.go new file mode 100644 index 000000000..fb717db91 --- /dev/null +++ b/paranoia.go @@ -0,0 +1,19 @@ +// Copyright 2020 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build paranoia + +package pilosa + +const paranoia = true diff --git a/row.go b/row.go index 5e2256841..98f5c9275 100644 --- a/row.go +++ b/row.go @@ -70,6 +70,8 @@ func (r *Row) Clone() (clone *Row) { clone = &Row{ Keys: keyClone, Attrs: attrClone, + Index: r.Index, + Field: r.Field, } for _, seg := range r.segments { @@ -299,6 +301,11 @@ func (r *Row) Union(others ...*Row) *Row { toProcess := make([]*rowSegment, 0, len(others)+1) var output []rowSegment for _, other := range others { + if paranoia { // nolint:staticcheck + // TODO I think there is a good check we can do here, but + // it's nontrivial to check whether two rows are + // compatible because of foreign indexes and such. + } if len(other.segments) > 0 { segments = append(segments, other.segments) } diff --git a/server.go b/server.go index 89d197fef..b664e4c87 100644 --- a/server.go +++ b/server.go @@ -657,7 +657,6 @@ func (s *Server) monitorResetTranslationSync() { case <-s.closing: return case <-s.resetTranslationSyncCh: - s.logger.Printf("holder translation sync beginning") s.wg.Add(1) go func() { // Obtaining this lock ensures that there is only