From 6c506f1138be3c40946ead79b78e3db03049c379 Mon Sep 17 00:00:00 2001 From: Jaden Weiss Date: Fri, 17 Jul 2020 17:49:11 -0400 Subject: [PATCH 1/2] fix rangeBetween when there are nonzero common upper bits and oversized rangeGT --- fragment.go | 19 +++++++++++++++---- fragment_internal_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/fragment.go b/fragment.go index 4121a4424..48fdff1f9 100644 --- a/fragment.go +++ b/fragment.go @@ -1395,6 +1395,7 @@ func (f *fragment) rangeGT(bitDepth uint, predicate int64, allowEquality bool) ( } func (f *fragment) rangeGTUnsigned(filter *Row, bitDepth uint, predicate uint64, allowEquality bool) (*Row, error) { +prep: switch { case predicate == 0 && allowEquality: // This query matches all possible values. @@ -1407,8 +1408,13 @@ func (f *fragment) rangeGTUnsigned(filter *Row, bitDepth uint, predicate uint64, matches = matches.Union(filter.Intersect(row)) } return matches, nil + case !allowEquality && uint(bits.Len64(predicate)) > bitDepth: + // The predicate is bigger than the BSI width, so nothing can be bigger. + return NewRow(), nil case allowEquality: predicate-- + allowEquality = false + goto prep } // Compare intermediate bits. @@ -1479,9 +1485,9 @@ func (f *fragment) rangeBetweenUnsigned(filter *Row, bitDepth uint, predicateMin } // Compare any upper bits which are equal. - firstDiff := int(msb(predicateMax^predicateMin)) - 1 + diffLen := bits.Len64(predicateMax ^ predicateMin) remaining := filter - for i := int(bitDepth - 1); i > firstDiff; i-- { + for i := int(bitDepth - 1); i >= diffLen; i-- { row := f.row(uint64(bsiOffsetBit + i)) switch (predicateMin >> uint(i)) & 1 { case 1: @@ -1491,12 +1497,17 @@ func (f *fragment) rangeBetweenUnsigned(filter *Row, bitDepth uint, predicateMin } } + // Clear the bits we just compared. + equalMask := (^uint64(0)) << diffLen + predicateMin &^= equalMask + predicateMax &^= equalMask + var err error - remaining, err = f.rangeGTUnsigned(remaining, uint(firstDiff+1), predicateMin, true) + remaining, err = f.rangeGTUnsigned(remaining, uint(diffLen), predicateMin, true) if err != nil { return nil, err } - remaining, err = f.rangeLTUnsigned(remaining, uint(firstDiff+1), predicateMax, true) + remaining, err = f.rangeLTUnsigned(remaining, uint(diffLen), predicateMax, true) if err != nil { return nil, err } diff --git a/fragment_internal_test.go b/fragment_internal_test.go index fa1ffbeec..f174e564d 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -706,6 +706,23 @@ func TestFragment_Range(t *testing.T) { } }) + t.Run("GTOversizeRegression", func(t *testing.T) { + f := mustOpenFragment("i", "f", viewStandard, 0, "") + defer f.Clean(t) + + if _, err := f.setValue(1, 2, 0); err != nil { + t.Fatal(err) + } else if _, err := f.setValue(2, 2, 1); err != nil { + t.Fatal(err) + } + + if b, err := f.rangeGTUnsigned(NewRow(1, 2), 2, 4, false); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(b.Columns(), []uint64{}) { + t.Fatalf("unepxected coulmns: %+v", b.Columns()) + } + }) + t.Run("BETWEEN", func(t *testing.T) { f := mustOpenFragment("i", "f", viewStandard, 0, "") defer f.Clean(t) @@ -753,6 +770,23 @@ func TestFragment_Range(t *testing.T) { t.Fatalf("unexpected columns: %+v", b.Columns()) } }) + + t.Run("BetweenCommonBitsRegression", func(t *testing.T) { + f := mustOpenFragment("i", "f", viewStandard, 0, "") + defer f.Clean(t) + + if _, err := f.setValue(1, 64, 0xf0); err != nil { + t.Fatal(err) + } else if _, err := f.setValue(2, 64, 0xf1); err != nil { + t.Fatal(err) + } + + if b, err := f.rangeBetweenUnsigned(NewRow(1, 2), 64, 0xf0, 0xf1); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(b.Columns(), []uint64{1, 2}) { + t.Fatalf("unepxected coulmns: %+v", b.Columns()) + } + }) } // benchmarkSetValues is a helper function to explore, very roughly, the cost From dcdcf1356f449d688146f8909e5c1e472f4ef577 Mon Sep 17 00:00:00 2001 From: Jaden Weiss Date: Mon, 20 Jul 2020 19:58:24 -0400 Subject: [PATCH 2/2] fix oversized rangeEQ --- fragment.go | 24 +++++++++++------------- fragment_internal_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/fragment.go b/fragment.go index 48fdff1f9..a8e5ad4fe 100644 --- a/fragment.go +++ b/fragment.go @@ -1230,12 +1230,18 @@ func (f *fragment) rangeEQ(bitDepth uint, predicate int64) (*Row, error) { // Start with set of columns with values set. b := f.row(bsiExistsBit) - // Filter to only positive/negative numbers. upredicate := absInt64(predicate) + if uint(bits.Len64(upredicate)) > bitDepth { + // Predicate is out of range. + return NewRow(), nil + } + + // Filter to only positive/negative numbers. + r := f.row(bsiSignBit) if predicate < 0 { - b = b.Intersect(f.row(bsiSignBit)) // only negatives + b = b.Intersect(r) // only negatives } else { - b = b.Difference(f.row(bsiSignBit)) // only positives + b = b.Difference(r) // only positives } // Filter any bits that don't match the current bit value. @@ -1308,18 +1314,10 @@ func (f *fragment) rangeLT(bitDepth uint, predicate int64, allowEquality bool) ( } } -// msb gives the 1-indexed position (counting from lsb) of the most -// significant bit. E.G. for 1 it would return 1, for 2 2, for 3 2, -// for 4 3, for 8 4, etc. -func msb(x uint64) uint { - lz := bits.LeadingZeros64(x) - return 64 - uint(lz) -} - // rangeLTUnsigned returns all bits LT/LTE the predicate without considering the sign bit. func (f *fragment) rangeLTUnsigned(filter *Row, bitDepth uint, predicate uint64, allowEquality bool) (*Row, error) { switch { - case msb(predicate) > bitDepth: + case uint(bits.Len64(predicate)) > bitDepth: fallthrough case predicate == (1<