-
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
Vcursor refactor #5982
Vcursor refactor #5982
Conversation
cf8eb1e
to
1a2968b
Compare
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
1a2968b
to
c4658bf
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
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.
Some nits that you can fix forward.
var _ Primitive = (*Send)(nil) | ||
|
||
// Send is an operator to send query to the specific keyspace, tabletType and destination | ||
type Send struct { |
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'd recommend a custom JSON marshaller that emits an additional fake "Opcode": "Send"
, which will make the output plan more readable (look at engine/limit.go).
|
||
// GetFields implements Primitive interface | ||
func (s *Send) GetFields(vcursor VCursor, bindVars map[string]*query.BindVariable) (*sqltypes.Result, error) { | ||
return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "not reachable") // TODO: systay - @sugu, is this correct? |
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.
Correct for now. We have to watch out if/when we allow these primitives to become composable.
|
||
// Send is an operator to send query to the specific keyspace, tabletType and destination | ||
type Send struct { | ||
// Keyspace specifies the keyspace to send the query to. |
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 also see that Send
is mostly a subset of Route
, but it's good to keep it separate for now.
e.updateQueryCounts("ShardDirect", "", "", int64(logStats.ShardQueries)) | ||
return result, err | ||
} | ||
func (e *Executor) handleExec(ctx context.Context, safeSession *SafeSession, sql string, bindVars map[string]*querypb.BindVariable, logStats *LogStats, stmtType sqlparser.StatementType) (*sqltypes.Result, error) { | ||
|
||
// V3 mode. |
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.
Remove comment.
@@ -194,6 +194,7 @@ func TestExecutorTransactionsNoAutoCommit(t *testing.T) { | |||
} | |||
} | |||
|
|||
//TODO - what about these? |
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.
Clarify comment.
In vitessio#5982 queries after `use keyspace[shard]` were changed to use a new "Send" engine primitive instead of v2 routing, which caused OLAP to stop working for queries targeted in that way. This PR fixes those OLAP queries. This PR does not implement GetFields, so prepared statements still aren't going to work with this kind of targeting. As a bonus this also fixes an issue where trying to start/rollback/commit a transaction in OLAP mode caused a panic due to plan.Instructions being nil in that case. Signed-off-by: David Weitzman <[email protected]>
Removed usage of resolver in handleExec by using bypass plan.