From b92add82cc6af429d55c53f1f3c92d1a6cbffbf4 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Fri, 26 Aug 2022 13:48:34 -0700 Subject: [PATCH 1/5] Removing dead code --- .../doltcore/table/typed/noms/range_reader.go | 39 ------------------- 1 file changed, 39 deletions(-) diff --git a/go/libraries/doltcore/table/typed/noms/range_reader.go b/go/libraries/doltcore/table/typed/noms/range_reader.go index 8c8cfdc839..9faac40fbd 100644 --- a/go/libraries/doltcore/table/typed/noms/range_reader.go +++ b/go/libraries/doltcore/table/typed/noms/range_reader.go @@ -20,8 +20,6 @@ import ( "fmt" "io" - "github.com/dolthub/go-mysql-server/sql" - "github.com/dolthub/dolt/go/libraries/doltcore/row" "github.com/dolthub/dolt/go/libraries/doltcore/schema" "github.com/dolthub/dolt/go/store/types" @@ -255,43 +253,6 @@ func (nrr *NomsRangeReader) Close(ctx context.Context) error { return nil } -// SqlRowFromTuples constructs a go-mysql-server/sql.Row from Noms tuples. -func SqlRowFromTuples(sch schema.Schema, key, val types.Tuple) (sql.Row, error) { - allCols := sch.GetAllCols() - colVals := make(sql.Row, allCols.Size()) - - keySl, err := key.AsSlice() - if err != nil { - return nil, err - } - valSl, err := val.AsSlice() - if err != nil { - return nil, err - } - - for _, sl := range []types.TupleValueSlice{keySl, valSl} { - var convErr error - err := row.IterPkTuple(sl, func(tag uint64, val types.Value) (stop bool, err error) { - if idx, ok := allCols.TagToIdx[tag]; ok { - col := allCols.GetByIndex(idx) - colVals[idx], convErr = col.TypeInfo.ConvertNomsValueToValue(val) - - if convErr != nil { - return false, err - } - } - - return false, nil - }) - - if err != nil { - return nil, err - } - } - - return sql.NewRow(colVals...), nil -} - type CardinalityCounter struct { key *types.Tuple value *types.Tuple From ed71542d22b6ff985ed1f84a9b2e39165a79e360 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Fri, 26 Aug 2022 13:50:54 -0700 Subject: [PATCH 2/5] Bug fix for Tuple.Compare to return consistent result across types when comparing against a null value. --- go/store/types/tuple.go | 9 +++++ go/store/types/tuple_test.go | 66 +++++++++++++++++++++++++++++------- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/go/store/types/tuple.go b/go/store/types/tuple.go index 78b73c14ab..2c24dba846 100644 --- a/go/store/types/tuple.go +++ b/go/store/types/tuple.go @@ -690,6 +690,15 @@ func (t Tuple) TupleCompare(nbf *NomsBinFormat, otherTuple Tuple) (int, error) { otherKind := otherDec.ReadKind() if kind != otherKind { + // If we are comparing any type to a null type, always evaluate + // the null value as greater than the non-null value. This is needed + // to keep null value ordering consistent in indexes. + if kind == NullKind { + return 1, nil + } else if otherKind == NullKind { + return -1, nil + } + return int(kind) - int(otherKind), nil } diff --git a/go/store/types/tuple_test.go b/go/store/types/tuple_test.go index c9bf154161..bd36161a2c 100644 --- a/go/store/types/tuple_test.go +++ b/go/store/types/tuple_test.go @@ -186,6 +186,46 @@ func TestTupleLess(t *testing.T) { []Value{UUID(uuid.MustParse(OneUUID)), String("abc")}, true, }, + { + []Value{UUID(uuid.MustParse(OneUUID)), String("abc")}, + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + true, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + []Value{UUID(uuid.MustParse(OneUUID)), String("abc")}, + false, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), Timestamp(time.Now())}, + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + true, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + []Value{UUID(uuid.MustParse(OneUUID)), Timestamp(time.Now())}, + false, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), Int(100)}, + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + true, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + []Value{UUID(uuid.MustParse(OneUUID)), Int(100)}, + false, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), Point{1, 1.0, 1.0}}, + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + true, + }, + { + []Value{UUID(uuid.MustParse(OneUUID)), NullValue}, + []Value{UUID(uuid.MustParse(OneUUID)), Point{1, 1.0, 1.0}}, + false, + }, } isLTZero := func(n int) bool { @@ -194,23 +234,25 @@ func TestTupleLess(t *testing.T) { nbf := Format_Default for _, test := range tests { - tpl1, err := NewTuple(nbf, test.vals1...) + t.Run("", func(t *testing.T) { + tpl1, err := NewTuple(nbf, test.vals1...) - require.NoError(t, err) + require.NoError(t, err) - tpl2, err := NewTuple(nbf, test.vals2...) - require.NoError(t, err) + tpl2, err := NewTuple(nbf, test.vals2...) + require.NoError(t, err) - actual, err := tpl1.Less(nbf, tpl2) - require.NoError(t, err) + actual, err := tpl1.Less(nbf, tpl2) + require.NoError(t, err) - if actual != test.expected { - t.Error("tpl1:", mustString(EncodedValue(context.Background(), tpl1)), "tpl2:", mustString(EncodedValue(context.Background(), tpl2)), "expected", test.expected, "actual:", actual) - } + if actual != test.expected { + t.Error("tpl1:", mustString(EncodedValue(context.Background(), tpl1)), "tpl2:", mustString(EncodedValue(context.Background(), tpl2)), "expected", test.expected, "actual:", actual) + } - res, err := tpl1.Compare(nbf, tpl2) - require.NoError(t, err) - require.Equal(t, actual, isLTZero(res)) + res, err := tpl1.Compare(nbf, tpl2) + require.NoError(t, err) + require.Equal(t, actual, isLTZero(res)) + }) } } From 06af2191d3206383b72b2c1ac4f06d82d733ca74 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Fri, 26 Aug 2022 14:05:26 -0700 Subject: [PATCH 3/5] Adding an enginetest to prevent regression --- .../doltcore/sqle/enginetest/dolt_queries.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index a72ead6313..7dbc06f00f 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go @@ -328,6 +328,24 @@ var DoltRevisionDbScripts = []queries.ScriptTest{ // DoltScripts are script tests specific to Dolt (not the engine in general), e.g. by involving Dolt functions. Break // this slice into others with good names as it grows. var DoltScripts = []queries.ScriptTest{ + { + Name: "test null filtering in secondary indexes (https://github.com/dolthub/dolt/issues/4199)", + SetUpScript: []string{ + "create table t (a int primary key auto_increment, d datetime, index index1 (d));", + "insert into t (d) values (NOW()), (NOW());", + "insert into t (d) values (NULL), (NULL);", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "select count(*) from t where d is not null", + Expected: []sql.Row{{2}}, + }, + { + Query: "select count(*) from t where d is null", + Expected: []sql.Row{{2}}, + }, + }, + }, { Name: "test backticks in index name (https://github.com/dolthub/dolt/issues/3776)", SetUpScript: []string{ From d637e37dc2330ad72d613bb6348daa6d1ef0e16e Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Fri, 26 Aug 2022 14:08:17 -0700 Subject: [PATCH 4/5] Revert "Removing dead code" This reverts commit b92add82cc6af429d55c53f1f3c92d1a6cbffbf4. --- .../doltcore/table/typed/noms/range_reader.go | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/go/libraries/doltcore/table/typed/noms/range_reader.go b/go/libraries/doltcore/table/typed/noms/range_reader.go index 9faac40fbd..8c8cfdc839 100644 --- a/go/libraries/doltcore/table/typed/noms/range_reader.go +++ b/go/libraries/doltcore/table/typed/noms/range_reader.go @@ -20,6 +20,8 @@ import ( "fmt" "io" + "github.com/dolthub/go-mysql-server/sql" + "github.com/dolthub/dolt/go/libraries/doltcore/row" "github.com/dolthub/dolt/go/libraries/doltcore/schema" "github.com/dolthub/dolt/go/store/types" @@ -253,6 +255,43 @@ func (nrr *NomsRangeReader) Close(ctx context.Context) error { return nil } +// SqlRowFromTuples constructs a go-mysql-server/sql.Row from Noms tuples. +func SqlRowFromTuples(sch schema.Schema, key, val types.Tuple) (sql.Row, error) { + allCols := sch.GetAllCols() + colVals := make(sql.Row, allCols.Size()) + + keySl, err := key.AsSlice() + if err != nil { + return nil, err + } + valSl, err := val.AsSlice() + if err != nil { + return nil, err + } + + for _, sl := range []types.TupleValueSlice{keySl, valSl} { + var convErr error + err := row.IterPkTuple(sl, func(tag uint64, val types.Value) (stop bool, err error) { + if idx, ok := allCols.TagToIdx[tag]; ok { + col := allCols.GetByIndex(idx) + colVals[idx], convErr = col.TypeInfo.ConvertNomsValueToValue(val) + + if convErr != nil { + return false, err + } + } + + return false, nil + }) + + if err != nil { + return nil, err + } + } + + return sql.NewRow(colVals...), nil +} + type CardinalityCounter struct { key *types.Tuple value *types.Tuple From 2946d5c0aa02888fecc91f51df89a6bce8f363ee Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Fri, 26 Aug 2022 15:45:31 -0700 Subject: [PATCH 5/5] Adding test assertions for IndexedJoin --- .../doltcore/sqle/enginetest/dolt_queries.go | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index 7dbc06f00f..c990e9f38d 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go @@ -331,7 +331,7 @@ var DoltScripts = []queries.ScriptTest{ { Name: "test null filtering in secondary indexes (https://github.com/dolthub/dolt/issues/4199)", SetUpScript: []string{ - "create table t (a int primary key auto_increment, d datetime, index index1 (d));", + "create table t (pk int primary key auto_increment, d datetime, index index1 (d));", "insert into t (d) values (NOW()), (NOW());", "insert into t (d) values (NULL), (NULL);", }, @@ -344,6 +344,31 @@ var DoltScripts = []queries.ScriptTest{ Query: "select count(*) from t where d is null", Expected: []sql.Row{{2}}, }, + { + // Test the null-safe equals operator + Query: "select count(*) from t where d <=> NULL", + Expected: []sql.Row{{2}}, + }, + { + // Test the null-safe equals operator + Query: "select count(*) from t where not(d <=> null)", + Expected: []sql.Row{{2}}, + }, + { + // Test an IndexedJoin + Query: "select count(ifnull(t.d, 1)) from t, t as t2 where t.d is not null and t.pk = t2.pk and t2.d is not null;", + Expected: []sql.Row{{2}}, + }, + { + // Test an IndexedJoin + Query: "select count(ifnull(t.d, 1)) from t, t as t2 where t.d is null and t.pk = t2.pk and t2.d is null;", + Expected: []sql.Row{{2}}, + }, + { + // Test an IndexedJoin + Query: "select count(ifnull(t.d, 1)) from t, t as t2 where t.d is null and t.pk = t2.pk and t2.d is not null;", + Expected: []sql.Row{{0}}, + }, }, }, {