-
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
Add support for SHOW (DATABASES|VITESS_SHARDS|VITESS_TABLETS) LIKE
#6750
Conversation
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
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.
I'll let @systay comment on parser tests, but there are existing tests for show vitess_tablets
which you could mimic and extend to test the query results.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
ping @systay |
In this case, it is better to delete most of the logging you added for debugging (except maybe keep the warning while ignoring a where clause). In the general case we want enough log to diagnose issues, but not so much that log files fill up too quickly. It's a difficult balancing act and we pretty much have to refine the logging on an ongoing basis. |
@@ -685,15 +685,29 @@ func (e *Executor) handleShow(ctx context.Context, safeSession *SafeSession, sql | |||
show.ShowTablesOpt.DbName = "" | |||
} | |||
sql = sqlparser.String(show) | |||
case sqlparser.KeywordString(sqlparser.DATABASES), "vitess_keyspaces", "keyspaces": | |||
case sqlparser.KeywordString(sqlparser.DATABASES), sqlparser.KeywordString(sqlparser.VITESS_KEYSPACES), sqlparser.KeywordString(sqlparser.KEYSPACES): |
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.
👍
go/vt/vtgate/executor.go
Outdated
@@ -702,13 +716,56 @@ func (e *Executor) handleShow(ctx context.Context, safeSession *SafeSession, sql | |||
RowsAffected: uint64(len(rows)), | |||
}, nil | |||
case "vitess_shards": |
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.
use sqlparser.KeywordString
here too?
go/vt/vtgate/executor.go
Outdated
@@ -727,7 +796,7 @@ func (e *Executor) handleShow(ctx context.Context, safeSession *SafeSession, sql | |||
RowsAffected: uint64(len(rows)), | |||
}, nil | |||
case "vitess_tablets": |
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.
and here
go/vt/vtgate/executor_test.go
Outdated
Rows: [][]sqltypes.Value{}, | ||
RowsAffected: 0, | ||
} | ||
if !reflect.DeepEqual(qr, wantqr) { |
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.
could you use utils.MustMatch
instead of DeepEqual
?
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.
Yep! (One day, I promise, I'll remember the first time 😅)
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.
@deepthi I noticed the rest of the assertions in this test case use reflect.DeepEqual
, do you want me to go ahead update those too?
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.
that will be great!
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.
Done! If that makes the diff a little gross to review both sets of changes at once let me know and I'll happily split them up :)
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…tablets Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay finally got to pull in those changes, works great. Thanks again for taking care of that! |
This addresses #6315 and then some.
Part 1
This PR updates the sql parser to add 4 new non-reserved keywords:
KEYSPACES
,VITESS_KEYSPACES
,VITESS_SHARDS
, andVITESS_TABLETS
.Then, we update the grammar to allow for an optional
LIKE
clause to be included when doing aSHOW DATABASES/KEYSPACES/VITESS_KEYSPACES
(this is the entirety of #6315, the rest is additional things which I can file a separate issue for).For filtering keyspaces, we create a regexp out of the LIKE clause, defaulting to
.*
, and only include keyspaces which match the regex.Part 2 - filtering shards and tablets
Next, this PR updates the grammar again, to create a
vitess_topo
nonterminal, which can be one of the VITESS_SHARDS / VITESS_TABLETS terminals. For both of these cases, we allow an option like or where. The LIKE clause currently has the following behavior:The where clause is currently unused, but in a future change I would like to add support for queries like:
SHOW VITESS_TABLETS WHERE keyspace = "mykeyspace" AND tablet_type != "MASTER"
(I have another branch with this, but the diff is quite large and I need to polish that quite a bit more).Tests
Used 101_initial_cluster.sh for all testing 😅 😬
Results
Some questions I have: