-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Allow specifying the hashing method for CreateLookupVindex #14157
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"fmt" | ||
"hash/fnv" | ||
"math" | ||
"slices" | ||
"sort" | ||
"strings" | ||
"sync" | ||
|
@@ -578,16 +579,33 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp | |
} | ||
|
||
// Validate input table | ||
if len(specs.Tables) != 1 { | ||
return nil, nil, nil, fmt.Errorf("exactly one table must be specified in the specs: %v", specs.Tables) | ||
if len(specs.Tables) < 1 || len(specs.Tables) > 2 { | ||
return nil, nil, nil, fmt.Errorf("one or two tables must be specified in the specs: %v", specs.Tables) | ||
} | ||
// Loop executes once. | ||
|
||
// Loop executes once or twice | ||
for k, ti := range specs.Tables { | ||
if len(ti.ColumnVindexes) != 1 { | ||
return nil, nil, nil, fmt.Errorf("exactly one ColumnVindex must be specified for the table: %v", specs.Tables) | ||
} | ||
sourceTableName = k | ||
sourceTable = ti | ||
if k != targetTableName { | ||
sourceTableName = k | ||
sourceTable = ti | ||
continue | ||
} | ||
|
||
var vindexCols []string | ||
if len(ti.ColumnVindexes[0].Columns) != 0 { | ||
vindexCols = ti.ColumnVindexes[0].Columns | ||
} else { | ||
if ti.ColumnVindexes[0].Column == "" { | ||
return nil, nil, nil, fmt.Errorf("at least one column must be specified in ColumnVindexes: %v", sourceTable.ColumnVindexes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be using ti.ColumnVindexes in the error. |
||
} | ||
vindexCols = []string{ti.ColumnVindexes[0].Column} | ||
} | ||
if !slices.Equal(vindexCols, vindexFromCols) { | ||
return nil, nil, nil, fmt.Errorf("columns in vindex don't match") | ||
} | ||
} | ||
|
||
// Validate input table and vindex consistency | ||
|
@@ -741,16 +759,22 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp | |
materializeQuery = buf.String() | ||
|
||
// Update targetVSchema | ||
var targetTable *vschemapb.Table | ||
targetTable := specs.Tables[targetTableName].CloneVT() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do a clone here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wanted to make sure it doesn’t end up modifying the original here from the user input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't change it though. And I feel like if we do change it, that might be what we want? I don't know why we'd want them to differ off the top of my head. I was gong to skip this part in the vtctldclient implementation. |
||
|
||
if targetVSchema.Sharded { | ||
// Choose a primary vindex type for target table based on source specs | ||
var targetVindexType string | ||
var targetVindex *vschemapb.Vindex | ||
for _, field := range tableSchema.TableDefinitions[0].Fields { | ||
if sourceVindexColumns[0] == field.Name { | ||
targetVindexType, err = vindexes.ChooseVindexForType(field.Type) | ||
if err != nil { | ||
return nil, nil, nil, err | ||
if targetTable != nil && len(targetTable.ColumnVindexes) > 0 { | ||
targetVindexType = targetTable.ColumnVindexes[0].Name | ||
} | ||
if targetVindexType == "" { | ||
targetVindexType, err = vindexes.ChooseVindexForType(field.Type) | ||
if err != nil { | ||
return nil, nil, nil, err | ||
} | ||
} | ||
targetVindex = &vschemapb.Vindex{ | ||
Type: targetVindexType, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should now instead specify the actual table name here rather than the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in the error message?