scriptoriumadmin/featurebase
1
0
Fork 0
mirror of https://github.com/featurebasedb/featurebase.git synced 2026-09-11 15:21:02 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Handle nonexistent shards in min/max decimal queries.

Browse source 
If a shard has never had any decimal values in it at all for a
field, the ValCount object returned has no DecimalVal, which could
cause a segfault if we don't check for it. Add a test case which
sporadically triggers that behavior (it's timing/luck related,
unfortunately), and then also fix it.
This commit is contained in:
Seebs 2020-04-09 14:58:22 -05:00 • committed by Matt Jaffee
parent 179cab91e7
commit 8e662d33a5
No known key found for this signature in database
GPG key ID: 08A3DFFF987B11BF
2 changed files with 43 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
executor.go
View file

@ -861,7 +861,6 @@ func (e *executor) executeGenericField(ctx context.Context, index string, c *pql
func (e *executor) executeMin(ctx context.Context, index string, c *pql.Call, shards []uint64, opt *execOptions) (ValCount, error) {
span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeMin")
defer span.Finish()
if field := c.Args["field"]; field == "" {
return ValCount{}, errors.New("Min(): field required")
}
@ -4124,7 +4123,7 @@ func (vc *ValCount) add(other ValCount) ValCount {
// smaller returns the smaller of the two ValCounts.
func (vc *ValCount) smaller(other ValCount) ValCount {
if vc.DecimalVal != nil {
if vc.DecimalVal != nil || other.DecimalVal != nil {
return vc.decimalSmaller(other)
} else if vc.FloatVal != 0 || other.FloatVal != 0 {
return vc.floatSmaller(other)
@ -4143,7 +4142,10 @@ func (vc *ValCount) smaller(other ValCount) ValCount {
}
func (vc *ValCount) decimalSmaller(other ValCount) ValCount {
if vc.Count == 0 || (other.DecimalVal.LessThan(*vc.DecimalVal) && other.Count > 0) {
if other.DecimalVal == nil {
return *vc
}
if vc.Count == 0 || vc.DecimalVal == nil || (other.DecimalVal.LessThan(*vc.DecimalVal) && other.Count > 0) {
return other
}
extra := int64(0)
@ -4172,7 +4174,7 @@ func (vc *ValCount) floatSmaller(other ValCount) ValCount {
// larger returns the larger of the two ValCounts.
func (vc *ValCount) larger(other ValCount) ValCount {
if vc.DecimalVal != nil {
if vc.DecimalVal != nil || other.DecimalVal != nil {
return vc.decimalLarger(other)
} else if vc.FloatVal != 0 || other.FloatVal != 0 {
return vc.floatLarger(other)
@ -4191,7 +4193,10 @@ func (vc *ValCount) larger(other ValCount) ValCount {
}
func (vc *ValCount) decimalLarger(other ValCount) ValCount {
if vc.Count == 0 || (other.DecimalVal.GreaterThan(*vc.DecimalVal) && other.Count > 0) {
if other.DecimalVal == nil {
return *vc
}
if vc.Count == 0 || vc.DecimalVal == nil || (other.DecimalVal.GreaterThan(*vc.DecimalVal) && other.Count > 0) {
return other
}
extra := int64(0)

34
executor_test.go
View file

@ -1512,6 +1512,38 @@ func TestExecutor_Execute_MinMax(t *testing.T) {
pql.Decimal{Value: -1150, Scale: 2},
},
}
// This extra field exists to make there be shards which are present,
// but have no decimal values set, to make sure they don't break
// the results.
if _, err := idx.CreateFieldIfNotExists("z", pilosa.OptFieldTypeDefault()); err != nil {
t.Fatal(err)
}
if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1, z=0)`}); err != nil {
t.Fatal(err)
} else if !res.Results[0].(bool) {
t.Fatalf("expected column changed")
}
// set things in other shards, that won't have decimal values
if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1234567, z=0)`}); err != nil {
t.Fatal(err)
} else if !res.Results[0].(bool) {
t.Fatalf("expected column changed")
}
if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2345678, z=0)`}); err != nil {
t.Fatal(err)
} else if !res.Results[0].(bool) {
t.Fatalf("expected column changed")
}
if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(3456789, z=0)`}); err != nil {
t.Fatal(err)
} else if !res.Results[0].(bool) {
t.Fatalf("expected column changed")
}
if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(4567890, z=0)`}); err != nil {
t.Fatal(err)
} else if !res.Results[0].(bool) {
t.Fatalf("expected column changed")
}
for i, test := range tests {
fld := fmt.Sprintf("f%d", i)
t.Run("MinMaxField_"+fld, func(t *testing.T) {
@ -1520,7 +1552,7 @@ func TestExecutor_Execute_MinMax(t *testing.T) {
}
if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: fmt.Sprintf(`
Set(10, %s=%s)
Set(6700000, %s=%s)
`, fld, test.set)}); err != nil {
t.Fatal(err)
}