Commit f1d6bc33 authored by craig[bot]'s avatar craig[bot]

Merge #50657 #50672

50657: kvserver: Replace multiTestContext with TestCluster in client_replica_gc_test r=lunevalex a=lunevalex

Makes progress on #8299

multiTestContext is legacy construct that is deprecated in favor of running
tests via TestCluster. This is one PR out of many to remove the usage of
multiTestContext in the client_replica_gc test cases.

Release note: none

50672: opt: add partial index implication logic for inexact atoms r=rytaft a=mgartner

### opt: rename partialidx/testdata/predicate/exact-atom to atom

This commit renames the `exact-atom` test file. Future commits will be
adding tests to this file that are not exact matches to atoms, which
would make the current file name confusing.

Release note: None

#### opt: add partial index implication logic for inexact atoms

This commit adds support for proving that a query filter implies a
partial index predicate when it contains expressions that do not exactly
match expressions in the predicate.

For example, the filter `a > 10` implies the predicate `a > 0`, because
all values greater than 10 are also greater than 0.

An atom A (an expression that is not a conjunction or disjunction)
implies another atom B when B contains A. To determine if an atom
contains another, we rely on the `constraints` package, which robustly
handles many types of operators (inequalities, `IN` expressions, etc.)
and operands (integers, strings, tuples, etc.).

Fixes #50218

Release note: None
Co-authored-by: default avatarAlex Lunev <[email protected]>
Co-authored-by: default avatarMarcus Gartner <[email protected]>
......@@ -201,7 +201,7 @@ and "atoms" (anything that is not an `AND` or `OR`) is as follows:
("=>" means "implies")
atom A => atom B if: A contains B
atom A => atom B if: B contains A
atom A => AND-expr B if: A => each of B's children
atom A => OR-expr B if: A => any of B's children
......
......@@ -17,11 +17,13 @@ import (
"testing"
"time"
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/storage/fs"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/errors"
)
......@@ -30,9 +32,10 @@ import (
// immediately cleaned up.
func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
defer leaktest.AfterTest(t)()
mtc := &multiTestContext{}
const numStores = 3
rangeID := roachpb.RangeID(1)
testKnobs := kvserver.StoreTestingKnobs{}
var tc *testcluster.TestCluster
// In this test, the Replica on the second Node is removed, and the test
// verifies that that Node adds this Replica to its RangeGCQueue. However,
......@@ -41,9 +44,7 @@ func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
// no GC will take place since the consistent RangeLookup hits the first
// Node. We use the TestingEvalFilter to make sure that the second Node
// waits for the first.
cfg := kvserver.TestStoreConfig(nil)
mtc.storeConfig = &cfg
mtc.storeConfig.TestingKnobs.EvalKnobs.TestingEvalFilter =
testKnobs.EvalKnobs.TestingEvalFilter =
func(filterArgs kvserverbase.FilterArgs) *roachpb.Error {
et, ok := filterArgs.Req.(*roachpb.EndTxnRequest)
if !ok || filterArgs.Sid != 2 {
......@@ -54,11 +55,12 @@ func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
return nil
}
testutils.SucceedsSoon(t, func() error {
r, err := mtc.stores[0].GetReplica(rangeID)
k := tc.ScratchRange(t)
desc, err := tc.LookupRange(k)
if err != nil {
return err
}
if _, ok := r.Desc().GetReplicaDescriptor(2); ok {
if _, ok := desc.GetReplicaDescriptor(2); ok {
return errors.New("expected second node gone from first node's known replicas")
}
return nil
......@@ -66,17 +68,32 @@ func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
return nil
}
defer mtc.Stop()
mtc.Start(t, numStores)
mtc.replicateRange(rangeID, 1, 2)
tc = testcluster.StartTestCluster(t, numStores,
base.TestClusterArgs{
ReplicationMode: base.ReplicationAuto,
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Store: &testKnobs,
},
},
},
)
defer tc.Stopper().Stop(context.Background())
k := tc.ScratchRange(t)
desc := tc.LookupRangeOrFatal(t, k)
ts := tc.Servers[1]
store, pErr := ts.Stores().GetStore(ts.GetFirstStoreID())
if pErr != nil {
t.Fatal(pErr)
}
{
repl1, err := mtc.stores[1].GetReplica(rangeID)
repl1, err := store.GetReplica(desc.RangeID)
if err != nil {
t.Fatal(err)
}
eng := mtc.engines[1]
eng := store.Engine()
// Put some bogus sideloaded data on the replica which we're about to
// remove. Then, at the end of the test, check that that sideloaded
......@@ -112,11 +129,11 @@ func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
}()
}
mtc.unreplicateRange(rangeID, 1)
desc = tc.RemoveReplicasOrFatal(t, k, tc.Target(1))
// Make sure the range is removed from the store.
testutils.SucceedsSoon(t, func() error {
if _, err := mtc.stores[1].GetReplica(rangeID); !testutils.IsError(err, "r[0-9]+ was not found") {
if _, err := store.GetReplica(desc.RangeID); !testutils.IsError(err, "r[0-9]+ was not found") {
return errors.Errorf("expected range removal: %v", err) // NB: errors.Wrapf(nil, ...) returns nil.
}
return nil
......@@ -127,40 +144,46 @@ func TestReplicaGCQueueDropReplicaDirect(t *testing.T) {
// removes a range from a store that no longer should have a replica.
func TestReplicaGCQueueDropReplicaGCOnScan(t *testing.T) {
defer leaktest.AfterTest(t)()
mtc := &multiTestContext{}
cfg := kvserver.TestStoreConfig(nil)
cfg.TestingKnobs.DisableEagerReplicaRemoval = true
cfg.Clock = nil // manual clock
mtc.storeConfig = &cfg
defer mtc.Stop()
mtc.Start(t, 3)
tc := testcluster.StartTestCluster(t, 3,
base.TestClusterArgs{
ReplicationMode: base.ReplicationAuto,
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Store: &kvserver.StoreTestingKnobs{
DisableEagerReplicaRemoval: true,
},
},
},
},
)
defer tc.Stopper().Stop(context.Background())
ts := tc.Servers[1]
store, pErr := ts.Stores().GetStore(ts.GetFirstStoreID())
if pErr != nil {
t.Fatal(pErr)
}
// Disable the replica gc queue to prevent direct removal of replica.
mtc.stores[1].SetReplicaGCQueueActive(false)
store.SetReplicaGCQueueActive(false)
rangeID := roachpb.RangeID(1)
mtc.replicateRange(rangeID, 1, 2)
mtc.unreplicateRange(rangeID, 1)
k := tc.ScratchRange(t)
desc := tc.RemoveReplicasOrFatal(t, k, tc.Target(1))
// Wait long enough for the direct replica GC to have had a chance and been
// discarded because the queue is disabled.
time.Sleep(10 * time.Millisecond)
if _, err := mtc.stores[1].GetReplica(rangeID); err != nil {
if _, err := store.GetReplica(desc.RangeID); err != nil {
t.Error("unexpected range removal")
}
// Enable the queue.
mtc.stores[1].SetReplicaGCQueueActive(true)
// Increment the clock's timestamp to make the replica GC queue process the range.
mtc.advanceClock(context.Background())
mtc.manualClock.Increment(int64(kvserver.ReplicaGCQueueInactivityThreshold + 1))
store.SetReplicaGCQueueActive(true)
// Make sure the range is removed from the store.
testutils.SucceedsSoon(t, func() error {
store := mtc.stores[1]
store.MustForceReplicaGCScanAndProcess()
if _, err := store.GetReplica(rangeID); !testutils.IsError(err, "r[0-9]+ was not found") {
if _, err := store.GetReplica(desc.RangeID); !testutils.IsError(err, "r[0-9]+ was not found") {
return errors.Errorf("expected range removal: %v", err) // NB: errors.Wrapf(nil, ...) returns nil.
}
return nil
......
......@@ -86,6 +86,20 @@ func (c *Columns) Equals(other *Columns) bool {
return true
}
// IsPrefixOf returns true if the columns in c are a prefix of the columns in
// other.
func (c *Columns) IsPrefixOf(other *Columns) bool {
if c.firstCol != other.firstCol || len(c.otherCols) > len(other.otherCols) {
return false
}
for i := range c.otherCols {
if c.otherCols[i] != other.otherCols[i] {
return false
}
}
return true
}
// IsStrictSuffixOf returns true if the columns in c are a strict suffix of the
// columns in other.
func (c *Columns) IsStrictSuffixOf(other *Columns) bool {
......
......@@ -228,6 +228,70 @@ func (c Constraint) String() string {
return b.String()
}
// Contains returns true if the constraint contains every span in the given
// constraint. The columns of the constraint must be a prefix of the columns of
// other.
func (c *Constraint) Contains(evalCtx *tree.EvalContext, other *Constraint) bool {
if !c.Columns.IsPrefixOf(&other.Columns) {
panic(errors.AssertionFailedf("columns must be a prefix of other columns"))
}
// An unconstrained constraint contains all constraints.
if c.IsUnconstrained() {
return true
}
// A contradiction is contained by all constraints.
if other.IsContradiction() {
return true
}
// Use variation on merge sort, because both sets of spans are ordered and
// non-overlapping.
left := &c.Spans
leftIndex := 0
right := &other.Spans
rightIndex := 0
keyCtx := MakeKeyContext(&c.Columns, evalCtx)
for leftIndex < left.Count() && rightIndex < right.Count() {
// If the current left span starts after the current right span, then
// the left span cannot contain the right span. Furthermore, no left
// spans can contain the current right spans.
cmpStart := left.Get(leftIndex).CompareStarts(&keyCtx, right.Get(rightIndex))
if cmpStart > 0 {
return false
}
cmpEnd := left.Get(leftIndex).CompareEnds(&keyCtx, right.Get(rightIndex))
// If the current left span end has the same end as the current right
// span, then the left span contains the right span. Move on to the next
// left and right spans.
if cmpEnd == 0 {
leftIndex++
rightIndex++
}
// If the current left span ends after the the current right span, then
// the left span contains the right span. The current left span could
// also contain other right spans, so only increment rightIndex.
if cmpEnd > 0 {
rightIndex++
}
// If the current left span ends before the current right span, then the
// left span cannot contain the right span, but other left spans could.
// Move on to the next left span.
if cmpEnd < 0 {
leftIndex++
}
}
// Return true if containment was proven for each span in right.
return rightIndex == right.Count()
}
// ContainsSpan returns true if the constraint contains the given span (or a
// span that contains it).
func (c *Constraint) ContainsSpan(evalCtx *tree.EvalContext, sp *Span) bool {
......
......@@ -155,6 +155,157 @@ func TestConstraintIntersect(t *testing.T) {
test(t, &evalCtx, &data.mangoStrawberry, &data.cherryRaspberry, expected)
}
func TestConstraintContains(t *testing.T) {
st := cluster.MakeTestingClusterSettings()
evalCtx := tree.MakeTestingEvalContext(st)
testData := []struct {
a string
b string
expected bool
}{
// Positive tests.
{
a: "/1: contradiction",
b: "/1: contradiction",
expected: true,
},
{
a: "/1: unconstrained",
b: "/1: unconstrained",
expected: true,
},
{
a: "/1: unconstrained",
b: "/1: contradiction",
expected: true,
},
{
a: "/1: unconstrained",
b: "/1: [/1 - /1]",
expected: true,
},
{
"/1: [/1 - /1]",
"/1: contradiction",
true,
},
{
"/1: [/1 - /1]",
"/1: [/1 - /1]",
true,
},
{
a: "/1: [/1 - /5]",
b: "/1: [/1 - /4]",
expected: true,
},
{
a: "/1: [/1 - /5]",
b: "/1: [/2 - /5]",
expected: true,
},
{
a: "/1: [/1 - /5]",
b: "/1: [/2 - /4]",
expected: true,
},
{
a: "/1: [/0 - /1] [/3 - /3]",
b: "/1: [/3 - /3]",
expected: true,
},
{
a: "/1: [/0 - /1] [/3 - /6]",
b: "/1: [/4 - /5]",
expected: true,
},
{
a: "/1: [/1 - /100]",
b: "/1: [/2 - /2] [/4 - /5] [/20 - /30]",
expected: true,
},
{
a: "/1: [/0 - /0] [/1 - /100] [/150 - /200]",
b: "/1: [/2 - /2] [/4 - /5] [/20 - /30]",
expected: true,
},
// Negative tests.
{
a: "/1: contradiction",
b: "/1: unconstrained",
expected: false,
},
{
a: "/1: contradiction",
b: "/1: [/1 - /1]",
expected: false,
},
{
a: "/1: [/1 - /1]",
b: "/1: [/2 - /2]",
expected: false,
},
{
a: "/1: [/1 - /2]",
b: "/1: [/0 - /2]",
expected: false,
},
{
a: "/1: [/1 - /2]",
b: "/1: [/1 - /3]",
expected: false,
},
{
a: "/1: [/0 - /0] [/1 - /1]",
b: "/1: [/0 - /0] [/2 - /2]",
expected: false,
},
{
a: "/1: [/0 - /0] [/2 - /3]",
b: "/1: [/0 - /0] [/1 - /3]",
expected: false,
},
{
a: "/1: [/0 - /0] [/1 - /2]",
b: "/1: [/0 - /0] [/1 - /3]",
expected: false,
},
{
a: "/1: [/0 - /1] [/3 - /3]",
b: "/1: [/2 - /2]",
expected: false,
},
{
a: "/1: [/1 - /100]",
b: "/1: [/2 - /2] [/4 - /5] [/90 - /110]",
expected: false,
},
{
a: "/1: [/0 - /0] [/1 - /100] [/120 - /120]",
b: "/1: [/2 - /2] [/4 - /5] [/90 - /110]",
expected: false,
},
}
for i, tc := range testData {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
ac := ParseConstraint(&evalCtx, tc.a)
bc := ParseConstraint(&evalCtx, tc.b)
res := ac.Contains(&evalCtx, &bc)
if res == tc.expected {
return
}
if tc.expected {
t.Errorf("%s should contain %s", ac, bc)
} else {
t.Errorf("%s should not contain %s", ac, bc)
}
})
}
}
func TestConstraintContainsSpan(t *testing.T) {
st := cluster.MakeTestingClusterSettings()
evalCtx := tree.MakeTestingEvalContext(st)
......
......@@ -11,6 +11,7 @@
package constraint
import (
"fmt"
"strconv"
"strings"
"time"
......@@ -26,7 +27,7 @@ import (
func ParseConstraint(evalCtx *tree.EvalContext, str string) Constraint {
s := strings.SplitN(str, ": ", 2)
if len(s) != 2 {
panic(str)
panic(fmt.Sprintf("invalid constraint format: %s", str))
}
var cols []opt.OrderingColumn
for _, v := range parseIntPath(s[0]) {
......@@ -41,13 +42,18 @@ func ParseConstraint(evalCtx *tree.EvalContext, str string) Constraint {
// parseSpans parses a list of spans with integer values like:
// "[/1 - /2] [/5 - /6]".
func parseSpans(evalCtx *tree.EvalContext, str string) Spans {
if str == "" {
if str == "" || str == "contradiction" {
return Spans{}
}
if str == "unconstrained" {
s := Spans{}
s.InitSingleSpan(&UnconstrainedSpan)
return s
}
s := strings.Split(str, " ")
// Each span has three pieces.
if len(s)%3 != 0 {
panic(str)
panic(fmt.Sprintf("invalid span format: %s", str))
}
var result Spans
for i := 0; i < len(s)/3; i++ {
......
......@@ -22,6 +22,16 @@ import (
"github.com/cockroachdb/errors"
)
// BuildConstraints returns a constraint.Set that represents the given scalar
// expression. A "tight" boolean is also returned which is true if the
// constraint is exactly equivalent to the expression.
func BuildConstraints(
e opt.ScalarExpr, md *opt.Metadata, evalCtx *tree.EvalContext,
) (_ *constraint.Set, tight bool) {
cb := constraintsBuilder{md: md, evalCtx: evalCtx}
return cb.buildConstraints(e)
}
// Convenience aliases to avoid the constraint prefix everywhere.
const includeBoundary = constraint.IncludeBoundary
const excludeBoundary = constraint.ExcludeBoundary
......
This diff is collapsed.
......@@ -106,7 +106,7 @@ func TestPredicateImplication(t *testing.T) {
d.Fatalf(t, "unexpected error while building predicate: %v\n", err)
}
remainingFilters, ok := partialidx.FiltersImplyPredicate(filters, pred, &f)
remainingFilters, ok := partialidx.FiltersImplyPredicate(filters, pred, &f, md, &evalCtx)
if !ok {
return "false"
}
......@@ -140,16 +140,16 @@ func BenchmarkPredicateImplication(b *testing.B) {
pred: "@1 >= 10",
},
{
name: "range-exact-match",
varTypes: "int, int",
filters: "@1 >= 10 AND @1 <= 100",
pred: "@1 >= 10 AND @1 <= 100",
name: "single-inexact-match",
varTypes: "int",
filters: "@1 >= 10",
pred: "@1 > 5",
},
{
name: "range-exact-match-reverse",
name: "range-inexact-match",
varTypes: "int, int",
filters: "@1 >= 10 AND @1 <= 100",
pred: "@1 >= 10 AND @1 <= 100",
filters: "@1 >= 10 AND @1 <= 90",
pred: "@1 > 0 AND @1 < 100",
},
{
name: "single-exact-match-extra-filters",
......@@ -158,16 +158,10 @@ func BenchmarkPredicateImplication(b *testing.B) {
pred: "@3 >= 10",
},
{
name: "single-exact-match-extra-filters-with-range",
name: "single-inexact-match-extra-filters",
varTypes: "int, int, int, int, int",
filters: "@1 < 0 AND @2 > 0 AND @3 >= 10 AND @3 <= 100 AND @4 = 4 AND @5 = 5",
pred: "@3 >= 10",
},
{
name: "range-exact-match-extra-filters",
varTypes: "int, int, int, int, int",
filters: "@1 < 0 AND @2 > 0 AND @3 >= 10 AND @3 <= 100 AND @4 = 4 AND @5 = 5",
pred: "@3 >= 10 AND @3 <= 100",
filters: "@1 < 0 AND @2 > 0 AND @3 >= 10 AND @4 = 4 AND @5 = 5",
pred: "@3 > 0",
},
{
name: "multi-column-and-exact-match",
......@@ -176,34 +170,28 @@ func BenchmarkPredicateImplication(b *testing.B) {
pred: "@1 >= 10 AND @2 = 'foo'",
},
{
name: "multi-column-or-exact-match",
name: "multi-column-and-inexact-match",
varTypes: "int, string",
filters: "@1 >= 10 OR @2 = 'foo'",
pred: "@1 >= 10 OR @2 = 'foo'",
filters: "@1 >= 10 AND @2 = 'foo'",
pred: "@1 >= 0 AND @2 IN ('foo', 'bar')",
},
{
name: "multi-column-and-exact-match-reverse",
name: "multi-column-or-exact-match",
varTypes: "int, string",
filters: "@1 >= 10 AND @2 = 'foo'",
pred: "@2 = 'foo' AND @1 >= 10",
filters: "@1 >= 10 OR @2 = 'foo'",
pred: "@1 >= 10 OR @2 = 'foo'",
},
{
name: "multi-column-and-exact-match-reverse",
name: "multi-column-or-exact-match-reverse",
varTypes: "int, string",
filters: "@1 >= 10 OR @2 = 'foo'",
pred: "@2 = 'foo' OR @1 >= 10",
},
{
name: "multi-column-and-exact-match-extra-filters",
varTypes: "int, int, int, int, string",
filters: "@1 < 0 AND @2 > 0 AND @3 >= 10 AND @4 = 4 AND @5 = 'foo'",
pred: "@2 > 0 AND @5 = 'foo'",
},
{
name: "multi-column-or-exact-match-extra-predicate",
varTypes: "int, int, int, int, string",
filters: "@2 > 0 OR @5 = 'foo'",
pred: "@1 < 0 OR @2 > 0 OR @3 >= 10 OR @4 = 4 OR @5 = 'foo'",
name: "multi-column-or-inexact-match",
varTypes: "int, string",
filters: "@1 >= 10 OR @2 = 'foo'",
pred: "@1 > 0 OR @2 IN ('foo', 'bar')",
},
{
name: "and-filters-do-not-imply-pred",
......@@ -221,8 +209,22 @@ func BenchmarkPredicateImplication(b *testing.B) {
// Generate a few test cases with many columns to show how performance
// scales with respect to the number of columns.
for _, n := range []int{10, 100} {
var tc testCase
tc.name = fmt.Sprintf("many-columns-%d", n)
tc := testCase{}
tc.name = fmt.Sprintf("many-columns-exact-match%d", n)
for i := 1; i <= n; i++ {
if i > 1 {
tc.varTypes += ", "
tc.filters += " AND "
tc.pred += " AND "
}
tc.varTypes += "int"
tc.filters += fmt.Sprintf("@%d = %d", i, i)
tc.pred += fmt.Sprintf("@%d = %d", i, i)
}
testCases = append(testCases, tc)
tc = testCase{}
tc.name = fmt.Sprintf("many-columns-inexact-match%d", n)
for i := 1; i <= n; i++ {
if i > 1 {
tc.varTypes += ", "
......@@ -230,8 +232,8 @@ func BenchmarkPredicateImplication(b *testing.B) {
tc.pred += " AND "
}
tc.varTypes += "int"
tc.filters += fmt.Sprintf("@%d=%d", i, i)
tc.pred += fmt.Sprintf("@%d=%d", i, i)
tc.filters += fmt.Sprintf("@%d > %d", i, i)
tc.pred += fmt.Sprintf("@%d >= %d", i, i)
}
testCases = append(testCases, tc)
}
......@@ -270,7 +272,7 @@ func BenchmarkPredicateImplication(b *testing.B) {
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, _ = partialidx.FiltersImplyPredicate(filters, pred, &f)
_, _ = partialidx.FiltersImplyPredicate(filters, pred, &f, md, &evalCtx)
}
})
}
......
......@@ -24,9 +24,16 @@ predtest vars=(bool, bool)
----
false
# Conjunction filters
predtest vars=(bool)
@1 AND true
=>
true AND @1
----
true
└── remaining filters: none
predtest vars=(bool, bool)
@1 AND @2
=>
......@@ -107,6 +114,14 @@ predtest vars=(int)
true
└── remaining filters: none
predtest vars=(int)
@1 > 20 AND @1 < 80
=>
@1 > 10 AND @1 < 100
----
true
└── remaining filters: (@1 > 20) AND (@1 < 80)
predtest vars=(int, bool)
@1 > 10 AND @2 AND @1 < 100
=>
......@@ -115,11 +130,34 @@ predtest vars=(int, bool)
true
└── remaining filters: @2
predtest vars=(int, bool)
@1 > 20 AND @2 AND @1 < 80
=>
@1 > 10 AND @1 < 100
----
true
└── remaining filters: ((@1 > 20) AND (@1 < 80)) AND @2
predtest vars=(int)
@1 > 10 AND @1 < 90
=>
@1 > 0 AND @1 < 100
----
true
└── remaining filters: (@1 > 10) AND (@1 < 90)
predtest vars=(int)
@1 > 10 AND @1 < 100
=>
@1 > 20 AND @1 < 80
----
false
predtest vars=(int, bool)
@1 > 10 AND @2 AND @1 < 100
=>
@1 > 20 AND @1 < 80
----
false
predtest vars=(int, bool)
......@@ -139,6 +177,14 @@ predtest vars=(bool)
true
└── remaining filters: none