From ff8d4481b0d5469edc7e181ed53bc8c069ab35e0 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 24 Jul 2023 22:30:50 +0530 Subject: [PATCH 1/3] remove usages of errHorizonNotPlanned Signed-off-by: Harshit Gangal --- .../planbuilder/operators/aggregation_pushing.go | 2 +- go/vt/vtgate/planbuilder/operators/aggregator.go | 2 +- .../planbuilder/operators/horizon_expanding.go | 2 +- .../planbuilder/operators/horizon_planning.go | 11 +---------- .../vtgate/planbuilder/operators/offset_planning.go | 11 +---------- go/vt/vtgate/planbuilder/operators/projection.go | 3 --- go/vt/vtgate/planbuilder/operators/table.go | 4 +++- go/vt/vtgate/planbuilder/testdata/tpch_cases.json | 4 ++-- .../planbuilder/testdata/unsupported_cases.json | 2 +- go/vt/vtgate/semantics/table_collector.go | 13 +++++++------ 10 files changed, 18 insertions(+), 36 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/aggregation_pushing.go b/go/vt/vtgate/planbuilder/operators/aggregation_pushing.go index 6fb5cc88be4..d724120423f 100644 --- a/go/vt/vtgate/planbuilder/operators/aggregation_pushing.go +++ b/go/vt/vtgate/planbuilder/operators/aggregation_pushing.go @@ -559,7 +559,7 @@ func (ab *aggBuilder) handleAggr(ctx *plancontext.PlanningContext, aggr Aggr) er // we are not going to see values multiple times, so we don't need to multiply with the count(*) from the other side return ab.handlePushThroughAggregation(ctx, aggr) default: - return errHorizonNotPlanned() + return vterrors.VT12001(fmt.Sprintf("aggregation not planned: %s", aggr.OpCode.String())) } } diff --git a/go/vt/vtgate/planbuilder/operators/aggregator.go b/go/vt/vtgate/planbuilder/operators/aggregator.go index ce6b9cc1912..25591f49f09 100644 --- a/go/vt/vtgate/planbuilder/operators/aggregator.go +++ b/go/vt/vtgate/planbuilder/operators/aggregator.go @@ -356,7 +356,7 @@ func (a *Aggregator) addIfAggregationColumn(ctx *plancontext.PlanningContext, co if _, srcIsAlsoAggr := a.Source.(*Aggregator); srcIsAlsoAggr { return 0, vterrors.VT12001("aggregation on top of aggregation not supported") } - return -1, vterrors.VT13001(fmt.Sprintf("aggregation column on wrong index: want: %d, got: %d", colIdx, offset)) + return -1, vterrors.VT12001(fmt.Sprintf("complex aggregation expression: %s", sqlparser.String(aggr.Original))) } a.Source = newSrc diff --git a/go/vt/vtgate/planbuilder/operators/horizon_expanding.go b/go/vt/vtgate/planbuilder/operators/horizon_expanding.go index f12015e7d7c..36891288b95 100644 --- a/go/vt/vtgate/planbuilder/operators/horizon_expanding.go +++ b/go/vt/vtgate/planbuilder/operators/horizon_expanding.go @@ -192,7 +192,7 @@ func createProjectionWithoutAggr(qp *QueryProjection, src ops.Operator) (*Projec aggr, ok := expr.(sqlparser.AggrFunc) if !ok { // need to add logic to extract aggregations and pushed them to the top level - return nil, errHorizonNotPlanned() + return nil, vterrors.VT12001(fmt.Sprintf("unsupported aggregation expression: %s", sqlparser.String(expr))) } expr = aggr.GetArg() if expr == nil { diff --git a/go/vt/vtgate/planbuilder/operators/horizon_planning.go b/go/vt/vtgate/planbuilder/operators/horizon_planning.go index dd6d5bc8898..63558d3be78 100644 --- a/go/vt/vtgate/planbuilder/operators/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/operators/horizon_planning.go @@ -131,16 +131,7 @@ func optimizeHorizonPlanning(ctx *plancontext.PlanningContext, root ops.Operator } } - newOp, err := rewrite.FixedPointBottomUp(root, TableID, visitor, stopAtRoute) - if err != nil { - if vterr, ok := err.(*vterrors.VitessError); ok && vterr.ID == "VT13001" { - // we encountered a bug. let's try to back out - return nil, errHorizonNotPlanned() - } - return nil, err - } - - return newOp, nil + return rewrite.FixedPointBottomUp(root, TableID, visitor, stopAtRoute) } func pushOrExpandHorizon(ctx *plancontext.PlanningContext, in *Horizon) (ops.Operator, *rewrite.ApplyResult, error) { diff --git a/go/vt/vtgate/planbuilder/operators/offset_planning.go b/go/vt/vtgate/planbuilder/operators/offset_planning.go index b94252d1af6..f4762404bba 100644 --- a/go/vt/vtgate/planbuilder/operators/offset_planning.go +++ b/go/vt/vtgate/planbuilder/operators/offset_planning.go @@ -49,16 +49,7 @@ func planOffsets(ctx *plancontext.PlanningContext, root ops.Operator) (ops.Opera return in, rewrite.SameTree, nil } - op, err := rewrite.TopDown(root, TableID, visitor, stopAtRoute) - if err != nil { - if vterr, ok := err.(*vterrors.VitessError); ok && vterr.ID == "VT13001" { - // we encountered a bug. let's try to back out - return nil, errHorizonNotPlanned() - } - return nil, err - } - - return op, nil + return rewrite.TopDown(root, TableID, visitor, stopAtRoute) } func fetchByOffset(e sqlparser.SQLNode) bool { diff --git a/go/vt/vtgate/planbuilder/operators/projection.go b/go/vt/vtgate/planbuilder/operators/projection.go index ff5a43a8c35..f99b6456670 100644 --- a/go/vt/vtgate/planbuilder/operators/projection.go +++ b/go/vt/vtgate/planbuilder/operators/projection.go @@ -86,9 +86,6 @@ func createSimpleProjection(ctx *plancontext.PlanningContext, qp *QueryProjectio } for _, e := range qp.SelectExprs { - if _, isStar := e.Col.(*sqlparser.StarExpr); isStar { - return nil, errHorizonNotPlanned() - } ae, err := e.GetAliasedExpr() if err != nil { return nil, err diff --git a/go/vt/vtgate/planbuilder/operators/table.go b/go/vt/vtgate/planbuilder/operators/table.go index 2409789d0bf..955120adf87 100644 --- a/go/vt/vtgate/planbuilder/operators/table.go +++ b/go/vt/vtgate/planbuilder/operators/table.go @@ -17,6 +17,8 @@ limitations under the License. package operators import ( + "fmt" + "vitess.io/vitess/go/slices2" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" @@ -104,7 +106,7 @@ func (to *Table) TablesUsed() []string { func addColumn(ctx *plancontext.PlanningContext, op ColNameColumns, e sqlparser.Expr) (int, error) { col, ok := e.(*sqlparser.ColName) if !ok { - return 0, vterrors.VT13001("cannot push this expression to a table/vindex: %s", sqlparser.String(e)) + return 0, vterrors.VT12001(fmt.Sprintf("cannot add '%s' expression to a table/vindex", sqlparser.String(e))) } sqlparser.RemoveKeyspaceFromColName(col) cols := op.GetColNames() diff --git a/go/vt/vtgate/planbuilder/testdata/tpch_cases.json b/go/vt/vtgate/planbuilder/testdata/tpch_cases.json index 5f0e6f93840..add3264c32e 100644 --- a/go/vt/vtgate/planbuilder/testdata/tpch_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/tpch_cases.json @@ -746,12 +746,12 @@ { "comment": "TPC-H query 8", "query": "select o_year, sum(case when nation = 'BRAZIL' then volume else 0 end) / sum(volume) as mkt_share from ( select extract(year from o_orderdate) as o_year, l_extendedprice * (1 - l_discount) as volume, n2.n_name as nation from part, supplier, lineitem, orders, customer, nation n1, nation n2, region where p_partkey = l_partkey and s_suppkey = l_suppkey and l_orderkey = o_orderkey and o_custkey = c_custkey and c_nationkey = n1.n_nationkey and n1.n_regionkey = r_regionkey and r_name = 'AMERICA' and s_nationkey = n2.n_nationkey and o_orderdate between date '1995-01-01' and date('1996-12-31') and p_type = 'ECONOMY ANODIZED STEEL' ) as all_nations group by o_year order by o_year", - "plan": "VT12001: unsupported: in scatter query: complex aggregate expression" + "plan": "VT12001: unsupported: complex aggregation expression: sum(case when nation = 'BRAZIL' then volume else 0 end)" }, { "comment": "TPC-H query 9", "query": "select nation, o_year, sum(amount) as sum_profit from ( select n_name as nation, extract(year from o_orderdate) as o_year, l_extendedprice * (1 - l_discount) - ps_supplycost * l_quantity as amount from part, supplier, lineitem, partsupp, orders, nation where s_suppkey = l_suppkey and ps_suppkey = l_suppkey and ps_partkey = l_partkey and p_partkey = l_partkey and o_orderkey = l_orderkey and s_nationkey = n_nationkey and p_name like '%green%' ) as profit group by nation, o_year order by nation, o_year desc", - "plan": "VT12001: unsupported: aggregation on columns from different sources" + "plan": "VT12001: unsupported: complex aggregation expression: sum(amount) as sum_profit" }, { "comment": "TPC-H query 10", diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index 0610202bead..40feb41c530 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -262,7 +262,7 @@ { "comment": "select func(keyspace_id) from user_index where id = :id", "query": "select func(keyspace_id) from user_index where id = :id", - "plan": "VT12001: unsupported: expression on results of a vindex function" + "plan": "VT12001: unsupported: cannot add 'func(keyspace_id)' expression to a table/vindex" }, { "comment": "delete with multi-table targets", diff --git a/go/vt/vtgate/semantics/table_collector.go b/go/vt/vtgate/semantics/table_collector.go index bf7f5ae74f4..b1da4893187 100644 --- a/go/vt/vtgate/semantics/table_collector.go +++ b/go/vt/vtgate/semantics/table_collector.go @@ -17,6 +17,7 @@ limitations under the License. package semantics import ( + querypb "vitess.io/vitess/go/vt/proto/query" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" @@ -106,12 +107,12 @@ func (tc *tableCollector) up(cursor *sqlparser.Cursor) error { func newVindexTable(t sqlparser.IdentifierCS) *vindexes.Table { vindexCols := []vindexes.Column{ - {Name: sqlparser.NewIdentifierCI("id")}, - {Name: sqlparser.NewIdentifierCI("keyspace_id")}, - {Name: sqlparser.NewIdentifierCI("range_start")}, - {Name: sqlparser.NewIdentifierCI("range_end")}, - {Name: sqlparser.NewIdentifierCI("hex_keyspace_id")}, - {Name: sqlparser.NewIdentifierCI("shard")}, + {Name: sqlparser.NewIdentifierCI("id"), Type: querypb.Type_VARBINARY}, + {Name: sqlparser.NewIdentifierCI("keyspace_id"), Type: querypb.Type_VARBINARY}, + {Name: sqlparser.NewIdentifierCI("range_start"), Type: querypb.Type_VARBINARY}, + {Name: sqlparser.NewIdentifierCI("range_end"), Type: querypb.Type_VARBINARY}, + {Name: sqlparser.NewIdentifierCI("hex_keyspace_id"), Type: querypb.Type_VARBINARY}, + {Name: sqlparser.NewIdentifierCI("shard"), Type: querypb.Type_VARBINARY}, } return &vindexes.Table{ From a0eb9d7da0275323067970208ca76f6b824f6faa Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 25 Jul 2023 13:21:23 +0530 Subject: [PATCH 2/3] add resuse existing column flag to route Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/operators/route.go | 25 ++++++------- .../planbuilder/testdata/aggr_cases.json | 35 +++++++++++++++++++ 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/route.go b/go/vt/vtgate/planbuilder/operators/route.go index 185032eae6c..b73fa51a510 100644 --- a/go/vt/vtgate/planbuilder/operators/route.go +++ b/go/vt/vtgate/planbuilder/operators/route.go @@ -543,21 +543,22 @@ func createProjection(src ops.Operator) (*Projection, error) { return proj, nil } -func (r *Route) AddColumn(ctx *plancontext.PlanningContext, expr *sqlparser.AliasedExpr, _, addToGroupBy bool) (ops.Operator, int, error) { +func (r *Route) AddColumn(ctx *plancontext.PlanningContext, expr *sqlparser.AliasedExpr, reuseExisting, addToGroupBy bool) (ops.Operator, int, error) { removeKeyspaceFromSelectExpr(expr) - // check if columns is already added. - cols, err := r.GetColumns() - if err != nil { - return nil, 0, err - } - colAsExpr := func(e *sqlparser.AliasedExpr) sqlparser.Expr { - return e.Expr - } - if offset, found := canReuseColumn(ctx, cols, expr.Expr, colAsExpr); found { - return r, offset, nil + if reuseExisting { + // check if columns is already added. + cols, err := r.GetColumns() + if err != nil { + return nil, 0, err + } + colAsExpr := func(e *sqlparser.AliasedExpr) sqlparser.Expr { + return e.Expr + } + if offset, found := canReuseColumn(ctx, cols, expr.Expr, colAsExpr); found { + return r, offset, nil + } } - // if column is not already present, we check if we can easily find a projection // or aggregation in our source that we can add to if ok, offset := addColumnToInput(r.Source, expr, addToGroupBy); ok { diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json index 6f84d838f34..071b3972f18 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json @@ -5877,5 +5877,40 @@ "user.user" ] } + }, + { + "comment": "aggregation on top of derived table with limit", + "query": "select count(val2), sum(val2) from (select id, val2 from user where val2 is null limit 2) as x", + "plan": { + "QueryType": "SELECT", + "Original": "select count(val2), sum(val2) from (select id, val2 from user where val2 is null limit 2) as x", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Scalar", + "Aggregates": "count(0) AS count(val2), sum(1) AS sum(val2)", + "Inputs": [ + { + "OperatorType": "Limit", + "Count": "INT64(2)", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select val2, val2 from (select id, val2 from `user` where 1 != 1) as x where 1 != 1", + "Query": "select val2, val2 from (select id, val2 from `user` where val2 is null) as x limit :__upper_limit", + "Table": "`user`" + } + ] + } + ] + }, + "TablesUsed": [ + "user.user" + ] + } } ] From ceef8f7a0828ac54da533c57e32b0ad781cd0eba Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 25 Jul 2023 14:46:36 +0530 Subject: [PATCH 3/3] update error message Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/operators/aggregator.go | 2 +- go/vt/vtgate/planbuilder/testdata/tpch_cases.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/aggregator.go b/go/vt/vtgate/planbuilder/operators/aggregator.go index 25591f49f09..b4210debd28 100644 --- a/go/vt/vtgate/planbuilder/operators/aggregator.go +++ b/go/vt/vtgate/planbuilder/operators/aggregator.go @@ -356,7 +356,7 @@ func (a *Aggregator) addIfAggregationColumn(ctx *plancontext.PlanningContext, co if _, srcIsAlsoAggr := a.Source.(*Aggregator); srcIsAlsoAggr { return 0, vterrors.VT12001("aggregation on top of aggregation not supported") } - return -1, vterrors.VT12001(fmt.Sprintf("complex aggregation expression: %s", sqlparser.String(aggr.Original))) + return -1, vterrors.VT12001(fmt.Sprintf("failed to plan aggregation on: %s", sqlparser.String(aggr.Original))) } a.Source = newSrc diff --git a/go/vt/vtgate/planbuilder/testdata/tpch_cases.json b/go/vt/vtgate/planbuilder/testdata/tpch_cases.json index add3264c32e..14482b7f78a 100644 --- a/go/vt/vtgate/planbuilder/testdata/tpch_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/tpch_cases.json @@ -746,12 +746,12 @@ { "comment": "TPC-H query 8", "query": "select o_year, sum(case when nation = 'BRAZIL' then volume else 0 end) / sum(volume) as mkt_share from ( select extract(year from o_orderdate) as o_year, l_extendedprice * (1 - l_discount) as volume, n2.n_name as nation from part, supplier, lineitem, orders, customer, nation n1, nation n2, region where p_partkey = l_partkey and s_suppkey = l_suppkey and l_orderkey = o_orderkey and o_custkey = c_custkey and c_nationkey = n1.n_nationkey and n1.n_regionkey = r_regionkey and r_name = 'AMERICA' and s_nationkey = n2.n_nationkey and o_orderdate between date '1995-01-01' and date('1996-12-31') and p_type = 'ECONOMY ANODIZED STEEL' ) as all_nations group by o_year order by o_year", - "plan": "VT12001: unsupported: complex aggregation expression: sum(case when nation = 'BRAZIL' then volume else 0 end)" + "plan": "VT12001: unsupported: failed to plan aggregation on: sum(case when nation = 'BRAZIL' then volume else 0 end)" }, { "comment": "TPC-H query 9", "query": "select nation, o_year, sum(amount) as sum_profit from ( select n_name as nation, extract(year from o_orderdate) as o_year, l_extendedprice * (1 - l_discount) - ps_supplycost * l_quantity as amount from part, supplier, lineitem, partsupp, orders, nation where s_suppkey = l_suppkey and ps_suppkey = l_suppkey and ps_partkey = l_partkey and p_partkey = l_partkey and o_orderkey = l_orderkey and s_nationkey = n_nationkey and p_name like '%green%' ) as profit group by nation, o_year order by nation, o_year desc", - "plan": "VT12001: unsupported: complex aggregation expression: sum(amount) as sum_profit" + "plan": "VT12001: unsupported: failed to plan aggregation on: sum(amount) as sum_profit" }, { "comment": "TPC-H query 10",