Skip to content

Properly support ignore_nulls in CreateLookupVindex #13913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattlord merged 6 commits into from
Sep 5, 2023
Merged

Properly support ignore_nulls in CreateLookupVindex #13913

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06fddff
Properly support ignore_nulls in CreateLookupVindex
mattlord Sep 3, 2023
1fac092
Improve unit test and add e2e test
mattlord Sep 4, 2023
01a3107
This is no longer needed
mattlord Sep 4, 2023
70b66ee
Kick CI
mattlord Sep 4, 2023
69694eb
Align ignore_nulls value handling
mattlord Sep 5, 2023
39e269f
Fix one more indentation spot in test vschema definition
mattlord Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions go/vt/vttablet/tabletserver/vstreamer/planbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ const (
GreaterThanEqual
// NotEqual is used to filter a comparable column if != specific value
NotEqual
// IsNotNull is used to filter a column if it is NULL
IsNotNull
)

// Filter contains opcodes for filtering.
Expand Down Expand Up @@ -224,6 +226,10 @@ func (plan *Plan) filter(values, result []sqltypes.Value, charsets []collations.
if !key.KeyRangeContains(filter.KeyRange, ksid) {
return false, nil
}
case IsNotNull:
if values[filter.ColNum].IsNull() {
return false, nil
}
default:
match, err := compare(filter.Opcode, values[filter.ColNum], filter.Value, charsets[filter.ColNum])
if err != nil {
Expand Down Expand Up @@ -550,6 +556,25 @@ func (plan *Plan) analyzeWhere(vschema *localVSchema, where *sqlparser.Where) er
if err := plan.analyzeInKeyRange(vschema, expr.Exprs); err != nil {
return err
}
case *sqlparser.IsExpr: // Needed for CreateLookupVindex with ignore_nulls
if expr.Right != sqlparser.IsNotNullOp {
return fmt.Errorf("unsupported constraint: %v", sqlparser.String(expr))
}
qualifiedName, ok := expr.Left.(*sqlparser.ColName)
if !ok {
return fmt.Errorf("unexpected: %v", sqlparser.String(expr))
}
if !qualifiedName.Qualifier.IsEmpty() {
return fmt.Errorf("unsupported qualifier for column: %v", sqlparser.String(qualifiedName))
}
colnum, err := findColumn(plan.Table, qualifiedName.Name)
if err != nil {
return err
}
plan.Filters = append(plan.Filters, Filter{
Opcode: IsNotNull,
ColNum: colnum,
})
default:
return fmt.Errorf("unsupported constraint: %v", sqlparser.String(expr))
}
Expand Down
39 changes: 27 additions & 12 deletions go/vt/wrangler/materializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"hash/fnv"
"math"
"sort"
"strconv"
"strings"
"sync"
"text/template"
Expand Down Expand Up @@ -504,12 +505,13 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp
// Important variables are pulled out here.
var (
// lookup vindex info
vindexName string
vindex *vschemapb.Vindex
targetKeyspace string
targetTableName string
vindexFromCols []string
vindexToCol string
vindexName string
vindex *vschemapb.Vindex
targetKeyspace string
targetTableName string
vindexFromCols []string
vindexToCol string
vindexIgnoreNulls bool

// source table info
sourceTableName string
Expand Down Expand Up @@ -560,6 +562,9 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp
if _, err := vindexes.CreateVindex(vindex.Type, vindexName, vindex.Params); err != nil {
return nil, nil, nil, err
}
if ignoreNullsStr, ok := vindex.Params["ignore_nulls"]; ok {
vindexIgnoreNulls, _ = strconv.ParseBool(ignoreNullsStr)
}

// Validate input table
if len(specs.Tables) != 1 {
Expand Down Expand Up @@ -696,21 +701,31 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp
buf = sqlparser.NewTrackedBuffer(nil)
buf.Myprintf("select ")
for i := range vindexFromCols {
buf.Myprintf("%v as %v, ", sqlparser.NewIdentifierCI(sourceVindexColumns[i]), sqlparser.NewIdentifierCI(vindexFromCols[i]))
buf.Myprintf("%s as %s, ", sqlparser.String(sqlparser.NewIdentifierCI(sourceVindexColumns[i])), sqlparser.String(sqlparser.NewIdentifierCI(vindexFromCols[i])))
}
if strings.EqualFold(vindexToCol, "keyspace_id") || strings.EqualFold(vindex.Type, "consistent_lookup_unique") || strings.EqualFold(vindex.Type, "consistent_lookup") {
buf.Myprintf("keyspace_id() as %v ", sqlparser.NewIdentifierCI(vindexToCol))
buf.Myprintf("keyspace_id() as %s ", sqlparser.String(sqlparser.NewIdentifierCI(vindexToCol)))
} else {
buf.Myprintf("%v as %v ", sqlparser.NewIdentifierCI(vindexToCol), sqlparser.NewIdentifierCI(vindexToCol))
buf.Myprintf("%s as %s ", sqlparser.String(sqlparser.NewIdentifierCI(vindexToCol)), sqlparser.String(sqlparser.NewIdentifierCI(vindexToCol)))
}
buf.Myprintf("from %s", sqlparser.String(sqlparser.NewIdentifierCS(sourceTableName)))
if vindexIgnoreNulls {
buf.Myprintf(" where ")
lastValIdx := len(vindexFromCols) - 1
for i := range vindexFromCols {
buf.Myprintf("%s is not null", sqlparser.String(sqlparser.NewIdentifierCI(vindexFromCols[i])))
if i != lastValIdx {
buf.Myprintf(" and ")
}
}
}
buf.Myprintf("from %v", sqlparser.NewIdentifierCS(sourceTableName))
if vindex.Owner != "" {
// Only backfill
buf.Myprintf(" group by ")
for i := range vindexFromCols {
buf.Myprintf("%v, ", sqlparser.NewIdentifierCI(vindexFromCols[i]))
buf.Myprintf("%s, ", sqlparser.String(sqlparser.NewIdentifierCI(vindexFromCols[i])))
}
buf.Myprintf("%v", sqlparser.NewIdentifierCI(vindexToCol))
buf.Myprintf("%s", sqlparser.String(sqlparser.NewIdentifierCI(vindexToCol)))

@dbussink dbussink Sep 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for switching this and other cases to explicit use sqlparser.String?

@mattlord mattlord Sep 4, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s to ensure that proper escaping/back ticking is done when needed.

@dbussink dbussink Sep 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok! Good to know!

@harshit-gangal harshit-gangal Sep 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this reason, I think we should create the AST struct and then convert it into the string. That is always safe and easy to follow.

}
materializeQuery = buf.String()

Expand Down
121 changes: 121 additions & 0 deletions go/vt/wrangler/materializer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,127 @@ func TestCreateCustomizedVindex(t *testing.T) {
}
}

func TestCreateLookupVindexIgnoreNulls(t *testing.T) {
ms := &vtctldatapb.MaterializeSettings{
SourceKeyspace: "ks",
TargetKeyspace: "ks",
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

env := newTestMaterializerEnv(t, ctx, ms, []string{"0"}, []string{"0"})
defer env.close()

specs := &vschemapb.Keyspace{
Vindexes: map[string]*vschemapb.Vindex{
"v": {
Type: "lookup_unique",
Params: map[string]string{
"table": "ks.lkp",
"from": "c1",
"to": "col2",
"ignore_nulls": "true",
},
Owner: "t1",
},
},
Tables: map[string]*vschemapb.Table{
"t1": {
ColumnVindexes: []*vschemapb.ColumnVindex{{
Name: "v",
Column: "col2",
}},
},
},
}
// Dummy sourceSchema
sourceSchema := "CREATE TABLE `t1` (\n" +
" `col1` int(11) NOT NULL AUTO_INCREMENT,\n" +
" `col2` int(11) DEFAULT NULL,\n" +
" PRIMARY KEY (`id`)\n" +
") ENGINE=InnoDB AUTO_INCREMENT=3 DEFAULT CHARSET=latin1"

vschema := &vschemapb.Keyspace{
Sharded: true,
Vindexes: map[string]*vschemapb.Vindex{
"hash": {
Type: "hash",
},
},
Tables: map[string]*vschemapb.Table{
"t1": {
ColumnVindexes: []*vschemapb.ColumnVindex{{
Name: "hash",
Column: "col1",
}},
},
},
}

wantKs := &vschemapb.Keyspace{
Sharded: true,
Vindexes: map[string]*vschemapb.Vindex{
"hash": {
Type: "hash",
},
"v": {
Type: "lookup_unique",
Params: map[string]string{
"table": "ks.lkp",
"from": "c1",
"to": "col2",
"write_only": "true",
"ignore_nulls": "true",
},
Owner: "t1",
},
},
Tables: map[string]*vschemapb.Table{
"t1": {
ColumnVindexes: []*vschemapb.ColumnVindex{{
Name: "hash",
Column: "col1",
}, {
Name: "v",
Column: "col2",
}},
},
"lkp": {
ColumnVindexes: []*vschemapb.ColumnVindex{{
Column: "c1",
Name: "hash",
}},
},
},
}
wantQuery := "select col2 as c1, col2 as col2 from t1 where c1 is not null group by c1, col2"

env.tmc.schema[ms.SourceKeyspace+".t1"] = &tabletmanagerdatapb.SchemaDefinition{
TableDefinitions: []*tabletmanagerdatapb.TableDefinition{{
Fields: []*querypb.Field{{
Name: "col1",
Type: querypb.Type_INT64,
}, {
Name: "col2",
Type: querypb.Type_INT64,
}},
Schema: sourceSchema,
}},
}
if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil {
t.Fatal(err)
}

ms, ks, _, err := env.wr.prepareCreateLookup(context.Background(), ms.SourceKeyspace, specs, false)
require.NoError(t, err)
if !proto.Equal(ks, wantKs) {
t.Errorf("unexpected keyspace value: got:\n%v, want\n%v", ks, wantKs)
}
require.NotNil(t, ms)
require.GreaterOrEqual(t, len(ms.TableSettings), 1)
require.Equal(t, ms.TableSettings[0].SourceExpression, wantQuery, "unexpected query")
}

func TestStopAfterCopyFlag(t *testing.T) {
ms := &vtctldatapb.MaterializeSettings{
SourceKeyspace: "ks",
Expand Down