From 8eaf17a0039ce1562ae895ed8785b183b9ef4a0a Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Tue, 16 Apr 2024 08:41:50 +0200 Subject: [PATCH] refactor: address review comments Signed-off-by: Andres Taylor --- go/mysql/sqlerror/sql_error.go | 1 - go/vt/vterrors/code.go | 1 - go/vt/vterrors/state.go | 1 - go/vt/vtgate/planbuilder/operators/offset_planning.go | 7 ++++--- go/vt/vtgate/planbuilder/operators/phases.go | 2 +- go/vt/vtgate/planbuilder/plan_test.go | 4 +--- go/vt/vtgate/vindexes/vschema.go | 2 +- 7 files changed, 7 insertions(+), 11 deletions(-) diff --git a/go/mysql/sqlerror/sql_error.go b/go/mysql/sqlerror/sql_error.go index 69cb397a015..bebd9e41ca7 100644 --- a/go/mysql/sqlerror/sql_error.go +++ b/go/mysql/sqlerror/sql_error.go @@ -244,7 +244,6 @@ var stateToMysqlCode = map[vterrors.State]mysqlCode{ vterrors.KillDeniedError: {num: ERKillDenied, state: SSUnknownSQLState}, vterrors.BadNullError: {num: ERBadNullError, state: SSConstraintViolation}, vterrors.InvalidGroupFuncUse: {num: ERInvalidGroupFuncUse, state: SSUnknownSQLState}, - vterrors.AggregateMustPushDown: {num: ERNotSupportedYet, state: SSUnknownSQLState}, } func getStateToMySQLState(state vterrors.State) mysqlCode { diff --git a/go/vt/vterrors/code.go b/go/vt/vterrors/code.go index a0d2b301aec..ffce4fc553d 100644 --- a/go/vt/vterrors/code.go +++ b/go/vt/vterrors/code.go @@ -58,7 +58,6 @@ var ( VT03030 = errorWithState("VT03030", vtrpcpb.Code_INVALID_ARGUMENT, WrongValueCountOnRow, "lookup column count does not match value count with the row (columns, count): (%v, %d)", "The number of columns you want to insert do not match the number of columns of your SELECT query.") VT03031 = errorWithoutState("VT03031", vtrpcpb.Code_INVALID_ARGUMENT, "EXPLAIN is only supported for single keyspace", "EXPLAIN has to be sent down as a single query to the underlying MySQL, and this is not possible if it uses tables from multiple keyspaces") VT03032 = errorWithState("VT03032", vtrpcpb.Code_INVALID_ARGUMENT, NonUpdateableTable, "the target table %s of the UPDATE is not updatable", "You cannot update a table that is not a real MySQL table.") - VT03033 = errorWithState("VT03033", vtrpcpb.Code_INVALID_ARGUMENT, AggregateMustPushDown, "aggregate user-defined function %s must be pushed down to mysql", "The aggregate user-defined function must be pushed down to mysql and can't be evaluated on the vtgate. The query contains aggregation that can't be fully pushed down to MySQL.") VT05001 = errorWithState("VT05001", vtrpcpb.Code_NOT_FOUND, DbDropExists, "cannot drop database '%s'; database does not exists", "The given database does not exist; Vitess cannot drop it.") VT05002 = errorWithState("VT05002", vtrpcpb.Code_NOT_FOUND, BadDb, "cannot alter database '%s'; unknown database", "The given database does not exist; Vitess cannot alter it.") diff --git a/go/vt/vterrors/state.go b/go/vt/vterrors/state.go index 78724cda892..2b0ada0bc6d 100644 --- a/go/vt/vterrors/state.go +++ b/go/vt/vterrors/state.go @@ -49,7 +49,6 @@ const ( WrongArguments BadNullError InvalidGroupFuncUse - AggregateMustPushDown // failed precondition NoDB diff --git a/go/vt/vtgate/planbuilder/operators/offset_planning.go b/go/vt/vtgate/planbuilder/operators/offset_planning.go index eff97ab8f07..06bfd6b7da7 100644 --- a/go/vt/vtgate/planbuilder/operators/offset_planning.go +++ b/go/vt/vtgate/planbuilder/operators/offset_planning.go @@ -139,7 +139,8 @@ func addColumnsToInput(ctx *plancontext.PlanningContext, root Operator) Operator for _, aggr := range aggrOp.Aggregations { if aggr.OpCode == opcode.AggregateUDF { // we don't support UDFs in aggregation if it's still above a route - panic(vterrors.VT03033(sqlparser.String(aggr.Original.Expr))) + message := fmt.Sprintf("Aggregate UDF %s must be pushed down to MySQL", sqlparser.String(aggr.Original.Expr)) + panic(vterrors.VT12001(message)) } } return in, NoRewrite @@ -154,8 +155,8 @@ func addColumnsToInput(ctx *plancontext.PlanningContext, root Operator) Operator return TopDown(root, TableID, visitor, stopAtRoute) } -// pullDistinctFromUNION will pull out the distinct from a union operator -func pullDistinctFromUNION(_ *plancontext.PlanningContext, root Operator) Operator { +// isolateDistinctFromUnion will pull out the distinct from a union operator +func isolateDistinctFromUnion(_ *plancontext.PlanningContext, root Operator) Operator { visitor := func(in Operator, _ semantics.TableSet, isRoot bool) (Operator, *ApplyResult) { union, ok := in.(*Union) if !ok || !union.distinct { diff --git a/go/vt/vtgate/planbuilder/operators/phases.go b/go/vt/vtgate/planbuilder/operators/phases.go index 2fc3a5a044f..60e937a5b92 100644 --- a/go/vt/vtgate/planbuilder/operators/phases.go +++ b/go/vt/vtgate/planbuilder/operators/phases.go @@ -88,7 +88,7 @@ func (p Phase) shouldRun(s semantics.QuerySignature) bool { func (p Phase) act(ctx *plancontext.PlanningContext, op Operator) Operator { switch p { case pullDistinctFromUnion: - return pullDistinctFromUNION(ctx, op) + return isolateDistinctFromUnion(ctx, op) case delegateAggregation: return enableDelegateAggregation(ctx, op) case addAggrOrdering: diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 8ffc88bb606..5ac3cf8a7cc 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -586,9 +586,7 @@ func TestOtherPlanningFromFile(t *testing.T) { func loadSchema(t testing.TB, filename string, setCollation bool) *vindexes.VSchema { formal, err := vindexes.LoadFormal(locateFile(filename)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) vschema := vindexes.BuildVSchema(formal, sqlparser.NewTestParser()) require.NoError(t, err) for _, ks := range vschema.Keyspaces { diff --git a/go/vt/vtgate/vindexes/vschema.go b/go/vt/vtgate/vindexes/vschema.go index 49987aef4dc..8dc889fc848 100644 --- a/go/vt/vtgate/vindexes/vschema.go +++ b/go/vt/vtgate/vindexes/vschema.go @@ -425,7 +425,7 @@ func (vschema *VSchema) AddView(ksname, viewName, query string, parser *sqlparse return nil } -// AddTable adds a table to an existing keyspace in the VSchema. +// AddUDF adds a UDF to an existing keyspace in the VSchema. // It's only used from tests. func (vschema *VSchema) AddUDF(ksname, udfName string) error { ks, ok := vschema.Keyspaces[ksname]