Skip to content
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

Jump to bottom

UNION DISTINCT support on vtgate #7029

Merged
merged 18 commits into from
Nov 17, 2020
Merged

UNION DISTINCT support on vtgate #7029

merged 18 commits into from
Nov 17, 2020

Conversation

systay
Copy link
Collaborator

@systay systay commented Nov 12, 2020 •
edited
Loading

Includes #7016

@harshit-gangal
Copy link
Member

Tried on vitess cluster and found panic on the query

select 1 union select null union select 1.0 union select "1" union select 2 union select 2.0 from product;

E1113 15:04:29.040949  132267 server.go:281] mysql_server caught panic:
interface conversion: sqlparser.SelectStatement is *sqlparser.Union, not *sqlparser.Select
/usr/local/go/src/runtime/iface.go:255 (0x40b983)
	panicdottypeE: panic(&TypeAssertionError{iface, have, want, ""})
/usr/local/go/src/runtime/iface.go:265 (0x40b8c3)
	panicdottypeI: panicdottypeE(t, want, iface)
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/route.go:166 (0xbff14d)
	io/vitess/go/vt/vtgate/planbuilder.(*route).MakeDistinct: rb.Select.(*sqlparser.Select).Distinct = true
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/union.go:58 (0xc0f0ce)
	io/vitess/go/vt/vtgate/planbuilder.(*primitiveBuilder).processUnion: bldr, err := pb.bldr.MakeDistinct()
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/union.go:36 (0xc0ea75)
	io/vitess/go/vt/vtgate/planbuilder.buildUnionPlan: if err := pb.processUnion(union, nil); err != nil {
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/builder.go:302 (0xbe68a4)
	io/vitess/go/vt/vtgate/planbuilder.buildRoutePlan: return f(stmt, vschema)
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/builder.go:316 (0xbe75f1)
	io/vitess/go/vt/vtgate/planbuilder.createInstructionFor: return buildRoutePlan(stmt, vschema, buildUnionPlan)
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/builder.go:329 (0xbe7190)
	io/vitess/go/vt/vtgate/planbuilder.createInstructionFor: innerInstruction, err := createInstructionFor(query, stmt.Statement, vschema)
/home/harshit/git-repo/vitess/go/vt/vtgate/planbuilder/builder.go:285 (0xbe668f)
	io/vitess/go/vt/vtgate/planbuilder.BuildFromStmt: instruction, err := createInstructionFor(query, stmt, vschema)
/home/harshit/git-repo/vitess/go/vt/vtgate/executor.go:1380 (0xc66842)
	io/vitess/go/vt/vtgate.(*Executor).getPlan: plan, err := planbuilder.BuildFromStmt(query, statement, vcursor, bindVarNeeds)
/home/harshit/git-repo/vitess/go/vt/vtgate/plan_execute.go:54 (0xc6d213)
	io/vitess/go/vt/vtgate.(*Executor).newExecute: plan, err := e.getPlan(
/home/harshit/git-repo/vitess/go/vt/vtgate/executor.go:185 (0xc5a13a)
	io/vitess/go/vt/vtgate.(*Executor).execute: stmtType, qr, err := e.newExecute(ctx, safeSession, sql, bindVars, logStats)
/home/harshit/git-repo/vitess/go/vt/vtgate/executor.go:154 (0xc59e57)
	io/vitess/go/vt/vtgate.(*Executor).Execute: stmtType, result, err := e.execute(ctx, safeSession, sql, bindVars, logStats)
/home/harshit/git-repo/vitess/go/vt/vtgate/vtgate.go:261 (0xc8a4d1)
	io/vitess/go/vt/vtgate.(*VTGate).Execute: qr, err = vtg.executor.Execute(ctx, "Execute", NewSafeSession(session), sql, bindVariables)
/home/harshit/git-repo/vitess/go/vt/vtgate/plugin_mysql_server.go:217 (0xc6f3b1)
	io/vitess/go/vt/vtgate.(*vtgateHandler).ComQuery: session, result, err := vh.vtg.Execute(ctx, session, query, make(map[string]*querypb.BindVariable))
/home/harshit/git-repo/vitess/go/mysql/conn.go:1232 (0x8b9b16)
	io/vitess/go/mysql.(*Conn).execQuery: err := handler.ComQuery(c, query, func(qr *sqltypes.Result) error {
/home/harshit/git-repo/vitess/go/mysql/conn.go:1217 (0x8b9688)
	io/vitess/go/mysql.(*Conn).handleComQuery: res := c.execQuery(sql, handler, more)
/home/harshit/git-repo/vitess/go/mysql/conn.go:865 (0x8b686f)
	io/vitess/go/mysql.(*Conn).handleNextCommand: return c.handleComQuery(handler, data)
/home/harshit/git-repo/vitess/go/mysql/server.go:473 (0x8d8e8e)
	io/vitess/go/mysql.(*Listener).handle: kontinue := c.handleNextCommand(l.handler)
/usr/local/go/src/runtime/asm_amd64.s:1357 (0x4613a0)
	goexit: BYTE	$0x90	// NOP

harshit-gangal
harshit-gangal requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

blocking as found an issue.

systay added 15 commits November 16, 2020 10:18
@systay
cleaned up test
ce3e819 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
add skeleton for a Distinct Primitive
412334c 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
single columns tested
2ad28e6 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
added support for multiple columns
f56b7f0 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
added test with hash collisions
02445ce 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
typo
e71547f 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
added distinct plan builder
de1e60d 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
updated the builder interface to return the new builder when doing a …
aa9df89 
…MakeDistinct()

Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
use the new distinct primitive to solve UNION DISTINCT
7cf1b0b 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
added ORDER BY to route plan description
a283b0f 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
implemented StreamExecute distinct
394396d 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
make sure to check all values when comparing rows
0077bdf 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
added endtoend tests for UNION DISTINCT
d9a663f 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
formatting
49751a1 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
updated test assertion
a8cfb01 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
Check the SELECT type before casting
124dbde 
In some code we were assuming that the Select field of the route builder
always was a *sqlparser.Select struct. This is not a safe assumption to
make, so this commit checks the type of the field before using it.

Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
moved test to the correct file
c30acc5 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
improved union distinct end2end tests
c71b6a5 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay requested a review from sougou as a code owner November 16, 2020 14:21
harshit-gangal
harshit-gangal approved these changes Nov 17, 2020
View reviewed changes
go/vt/vtgate/planbuilder/route.go
sel := rb.Select.(*sqlparser.Select)
sel, ok := rb.Select.(*sqlparser.Select)
if !ok {
return nil, 0, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unexpected AST struct for query")
Copy link
Member

Choose a reason for hiding this comment

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

nit: add type to the error message

@systay systay merged commit f71480f into vitessio:master Nov 17, 2020
@systay systay deleted the union-distinct branch November 17, 2020 08:56
@askdba askdba added this to the v9.0 milestone Nov 27, 2020
@rafael rafael mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshit-gangal harshit-gangal harshit-gangal approved these changes

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@systay @harshit-gangal @askdba