LookupVindex: fix CLI to allow creating non-unique lookups with single column #17301
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… vindexes Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17301 +/- ##
==========================================
+ Coverage 67.41% 67.49% +0.08%
==========================================
Files 1574 1577 +3
Lines 253202 253408 +206
==========================================
+ Hits 170686 171029 +343
+ Misses 82516 82379 -137 ☔ View full report in Codecov by Sentry. |
rohit-nayak-ps
removed
NeedsWebsiteDocsUpdate
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
requested review from
GuptaManan100,
frouioui,
deepthi,
mattlord and
harshit-gangal
as code owners
rohit-nayak-ps
requested review from
deepthi
and removed request for
deepthi
rohit-nayak-ps
added
the
Backport to: release-19.0
rohit-nayak-ps
added
Backport to: release-20.0
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
This was referenced Dec 2, 2024
Closed
Closed
Closed
Closed
harshit-gangal
force-pushed
the
rohit/clv-remove-unnecessary-validation
branch
from
3dd5c4b to
933b2bf
Compare
harshit-gangal
requested review from
systay,
timvaillancourt,
derekperkins,
vmg,
ajm188 and
notfelineit
as code owners
rohit-nayak-ps
force-pushed
the
rohit/clv-remove-unnecessary-validation
branch
from
933b2bf to
3dd5c4b
Compare
mattlord
approved these changes
Dec 5, 2024
Comment on lines
50
to
51
| //err = vc.VtctldClient.ExecuteCommand("LookupVindex", "--name", vindexName, "--table-keyspace=product", "create", "--keyspace=customer", | ||
| // "--type=consistent_lookup", "--table-owner=customer", "--table-owner-columns=name,cid", "--ignore-nulls", "--tablet-types=PRIMARY") |
There was a problem hiding this comment.
Any reason not to remove this?
There was a problem hiding this comment.
I assumed that it is documenting what we want the command to look like. Another way to do it would be to provide the vtctldclient invocation that is equivalent to this, that might be a bit less verbose.
| create table t1( | ||
| c1 int, | ||
| c2 int, | ||
| val varchar(128), |
There was a problem hiding this comment.
Obviously a very minor nit, but the spacing is off here (maybe a mix of spaces and tabs).
| tablets := make(map[string]*cluster.VttabletProcess) | ||
| if _, err := vc.AddKeyspace(t, []*Cell{vc.Cells["zone1"]}, lookupClusterSpec.keyspaceName, "-80,80-", | ||
| lookupClusterSpec.vschema, lookupClusterSpec.schema, defaultReplicas, defaultRdonly, 200, nil); err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
We should use require.NoError instead as it's much easier to understand the failures (have a nice stack trace etc).
Comment on lines
104
to
105
| tabs := setupLookupIndexKeyspace(t) | ||
| _ = tabs |
There was a problem hiding this comment.
Any reason not to use _ = setupLookupIndexKeyspace(t) instead?
deepthi
approved these changes
Dec 8, 2024
Comment on lines
50
to
51
| //err = vc.VtctldClient.ExecuteCommand("LookupVindex", "--name", vindexName, "--table-keyspace=product", "create", "--keyspace=customer", | ||
| // "--type=consistent_lookup", "--table-owner=customer", "--table-owner-columns=name,cid", "--ignore-nulls", "--tablet-types=PRIMARY") |
There was a problem hiding this comment.
I assumed that it is documenting what we want the command to look like. Another way to do it would be to provide the vtctldclient invocation that is equivalent to this, that might be a bit less verbose.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
This was referenced Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There was an incorrect validation in
LookupIndexthat a non-unique vindex should have more than one columns. This PR removes that check.We also add a new e2e test specifically around lookup vindexes to validate this change and add a framework to test lookup vindexes more extensively in the future.
This bug has been around since the beginning, so we should backport this to supported versions.
Related Issue(s)
#17302
Checklist