From 246aef32616c80e194cd43385978977d15ba97d0 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 20 Jun 2024 14:41:12 +0530 Subject: [PATCH 1/7] test: add failing test case Signed-off-by: Manan Gupta --- .../aggregation/aggregation.test | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 go/test/endtoend/vtgate/vitess_tester/aggregation/aggregation.test diff --git a/go/test/endtoend/vtgate/vitess_tester/aggregation/aggregation.test b/go/test/endtoend/vtgate/vitess_tester/aggregation/aggregation.test new file mode 100644 index 00000000000..8861b9672b8 --- /dev/null +++ b/go/test/endtoend/vtgate/vitess_tester/aggregation/aggregation.test @@ -0,0 +1,50 @@ +CREATE TABLE `t1` +( + `id` int unsigned NOT NULL AUTO_INCREMENT, + `name` varchar(191) NOT NULL, + PRIMARY KEY (`id`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_unicode_ci; + +CREATE TABLE `t2` +( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `t1_id` int unsigned NOT NULL, + PRIMARY KEY (`id`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_unicode_ci; + +CREATE TABLE `t3` +( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `name` varchar(191) NOT NULL, + PRIMARY KEY (`id`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_unicode_ci; + +insert into t1 (id, name) +values (1, 'A'), + (2, 'B'), + (3, 'C'), + (4, 'D'); + +insert into t2 (id, t1_id) +values (1, 1), + (2, 2), + (3, 3); + +insert into t3 (id, name) +values (1, 'A'), + (2, 'B'); + +-- wait_authoritative t1 +-- wait_authoritative t2 +-- wait_authoritative t3 +select group_concat(t3.name SEPARATOR ', ') as "Group Name" +from t1 + join t2 on t1.id = t2.t1_id + left join t3 on t1.id = t3.id +group by t1.id; \ No newline at end of file From 2cf1bee6e9a55afed020f6da5c01995ec2d39dca Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 20 Jun 2024 15:26:46 +0530 Subject: [PATCH 2/7] feat: fix group concat with separator evaluation Signed-off-by: Manan Gupta --- go/vt/vtgate/engine/aggregations.go | 20 +++++-- go/vt/vtgate/engine/cached_size.go | 4 +- .../planbuilder/operator_transformers.go | 2 +- .../operators/aggregation_pushing_helper.go | 2 +- .../planbuilder/testdata/aggr_cases.json | 60 +++++++++++++++++++ 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/engine/aggregations.go b/go/vt/vtgate/engine/aggregations.go index 4673a2717e5..35df2704fea 100644 --- a/go/vt/vtgate/engine/aggregations.go +++ b/go/vt/vtgate/engine/aggregations.go @@ -43,7 +43,7 @@ type AggregateParams struct { Type evalengine.Type Alias string `json:",omitempty"` - Expr sqlparser.Expr + Func sqlparser.AggrFunc Original *sqlparser.AliasedExpr // This is based on the function passed in the select expression and @@ -255,8 +255,9 @@ func (a *aggregatorScalar) reset() { } type aggregatorGroupConcat struct { - from int - type_ sqltypes.Type + from int + type_ sqltypes.Type + separator []byte concat []byte n int @@ -267,7 +268,7 @@ func (a *aggregatorGroupConcat) add(row []sqltypes.Value) error { return nil } if a.n > 0 { - a.concat = append(a.concat, ',') + a.concat = append(a.concat, a.separator...) } a.concat = append(a.concat, row[a.from].Raw()...) a.n++ @@ -434,7 +435,16 @@ func newAggregation(fields []*querypb.Field, aggregates []*AggregateParams) (agg ag = &aggregatorScalar{from: aggr.Col} case AggregateGroupConcat: - ag = &aggregatorGroupConcat{from: aggr.Col, type_: targetType} + separator := []byte{','} + gcFunc := aggr.Func.(*sqlparser.GroupConcatExpr) + if gcFunc.Separator != "" { + separator = []byte(gcFunc.Separator) + } + ag = &aggregatorGroupConcat{ + from: aggr.Col, + type_: targetType, + separator: separator, + } default: panic("BUG: unexpected Aggregation opcode") diff --git a/go/vt/vtgate/engine/cached_size.go b/go/vt/vtgate/engine/cached_size.go index e65ff61a9f6..df23f14f6f5 100644 --- a/go/vt/vtgate/engine/cached_size.go +++ b/go/vt/vtgate/engine/cached_size.go @@ -41,8 +41,8 @@ func (cached *AggregateParams) CachedSize(alloc bool) int64 { size += cached.Type.CachedSize(false) // field Alias string size += hack.RuntimeAllocSize(int64(len(cached.Alias))) - // field Expr vitess.io/vitess/go/vt/sqlparser.Expr - if cc, ok := cached.Expr.(cachedObject); ok { + // field Func vitess.io/vitess/go/vt/sqlparser.AggrFunc + if cc, ok := cached.Func.(cachedObject); ok { size += cc.CachedSize(true) } // field Original *vitess.io/vitess/go/vt/sqlparser.AliasedExpr diff --git a/go/vt/vtgate/planbuilder/operator_transformers.go b/go/vt/vtgate/planbuilder/operator_transformers.go index bec5cd28bb5..9d251fde8f2 100644 --- a/go/vt/vtgate/planbuilder/operator_transformers.go +++ b/go/vt/vtgate/planbuilder/operator_transformers.go @@ -308,7 +308,7 @@ func transformAggregator(ctx *plancontext.PlanningContext, op *operators.Aggrega return nil, vterrors.VT12001(fmt.Sprintf("in scatter query: aggregation function '%s'", sqlparser.String(aggr.Original))) } aggrParam := engine.NewAggregateParam(aggr.OpCode, aggr.ColOffset, aggr.Alias, ctx.VSchema.Environment().CollationEnv()) - aggrParam.Expr = aggr.Func + aggrParam.Func = aggr.Func aggrParam.Original = aggr.Original aggrParam.OrigOpcode = aggr.OriginalOpCode aggrParam.WCol = aggr.WSOffset diff --git a/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go b/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go index 2c47f426695..a14e9243e6d 100644 --- a/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go +++ b/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go @@ -134,7 +134,7 @@ func (ab *aggBuilder) handleAggr(ctx *plancontext.PlanningContext, aggr Aggr) er return ab.handlePushThroughAggregation(ctx, aggr) case opcode.AggregateGroupConcat: f := aggr.Func.(*sqlparser.GroupConcatExpr) - if f.Distinct || len(f.OrderBy) > 0 || f.Separator != "" { + if f.Distinct || len(f.OrderBy) > 0 { panic("fail here") } // this needs special handling, currently aborting the push of function diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json index fb04edd6d44..758342f3b35 100644 --- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json @@ -824,6 +824,66 @@ ] } }, + { + "comment": "group concat with a separator needing evaluation on vtgate", + "query": "select group_concat(music.name SEPARATOR ', ') as `Group Name` from user join user_extra on user.id = user_extra.user_id left join music on user.id = music.id group by user.id;", + "plan": { + "QueryType": "SELECT", + "Original": "select group_concat(music.name SEPARATOR ', ') as `Group Name` from user join user_extra on user.id = user_extra.user_id left join music on user.id = music.id group by user.id;", + "Instructions": { + "OperatorType": "Aggregate", + "Variant": "Ordered", + "Aggregates": "group_concat(0) AS Group Name", + "GroupBy": "(1|2)", + "ResultColumns": 1, + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "LeftJoin", + "JoinColumnIndexes": "R:0,L:0,L:1", + "JoinVars": { + "user_id": 0 + }, + "TableName": "`user`, user_extra_music", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.id, weight_string(`user`.id) from `user`, user_extra where 1 != 1", + "OrderBy": "(0|1) ASC", + "Query": "select `user`.id, weight_string(`user`.id) from `user`, user_extra where `user`.id = user_extra.user_id order by `user`.id asc", + "Table": "`user`, user_extra" + }, + { + "OperatorType": "Route", + "Variant": "EqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select music.`name` from music where 1 != 1", + "Query": "select music.`name` from music where music.id = :user_id", + "Table": "music", + "Values": [ + ":user_id" + ], + "Vindex": "music_user_map" + } + ] + } + ] + }, + "TablesUsed": [ + "user.music", + "user.user", + "user.user_extra" + ] + } + }, { "comment": "scatter aggregate group by column number", "query": "select col from user group by 1", From b6570e5f3447700c51fe56537cecd9c47921f9f7 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 20 Jun 2024 15:37:59 +0530 Subject: [PATCH 3/7] refactor: fix the error message to prevent vtgate crash Signed-off-by: Manan Gupta --- .../planbuilder/operators/aggregation_pushing_helper.go | 2 +- go/vt/vtgate/planbuilder/testdata/unsupported_cases.json | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go b/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go index a14e9243e6d..2fbf5c32311 100644 --- a/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go +++ b/go/vt/vtgate/planbuilder/operators/aggregation_pushing_helper.go @@ -135,7 +135,7 @@ func (ab *aggBuilder) handleAggr(ctx *plancontext.PlanningContext, aggr Aggr) er case opcode.AggregateGroupConcat: f := aggr.Func.(*sqlparser.GroupConcatExpr) if f.Distinct || len(f.OrderBy) > 0 { - panic("fail here") + panic(vterrors.VT12001("cannot evaluate group concat with distinct or order by")) } // this needs special handling, currently aborting the push of function // and later will try pushing the column instead. diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index a8fd082eb7b..38119ba936c 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -174,6 +174,11 @@ "query": "select id2 from user uu where id in (select id from user where id = uu.id and user.col in (select col from (select id from user_extra where user_id = 5) uu where uu.user_id = uu.id))", "plan": "VT12001: unsupported: correlated subquery is only supported for EXISTS" }, + { + "comment": "group concat with order by requiring evaluation at vtgate", + "query": "select group_concat(music.name ORDER BY 1 asc SEPARATOR ', ') as `Group Name` from user join user_extra on user.id = user_extra.user_id left join music on user.id = music.id group by user.id;", + "plan": "VT12001: unsupported: cannot evaluate group concat with distinct or order by" + }, { "comment": "outer and inner subquery route reference the same \"uu.id\" name\n# but they refer to different things. The first reference is to the outermost query,\n# and the second reference is to the innermost 'from' subquery.\n# changed to project all the columns from the derived tables.", "query": "select id2 from user uu where id in (select id from user where id = uu.id and user.col in (select col from (select col, id, user_id from user_extra where user_id = 5) uu where uu.user_id = uu.id))", From 0b0fd08dc27f64c9e8ca16f1c1417718fd049bf1 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 20 Jun 2024 18:31:11 +0530 Subject: [PATCH 4/7] test: fix tests Signed-off-by: Manan Gupta --- go/vt/vtgate/engine/ordered_aggregate_test.go | 9 +++++++-- go/vt/vtgate/engine/scalar_aggregation_test.go | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/engine/ordered_aggregate_test.go b/go/vt/vtgate/engine/ordered_aggregate_test.go index 3eaa63819e4..13da9beaf6c 100644 --- a/go/vt/vtgate/engine/ordered_aggregate_test.go +++ b/go/vt/vtgate/engine/ordered_aggregate_test.go @@ -22,6 +22,7 @@ import ( "fmt" "testing" + "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vtgate/evalengine" "github.com/stretchr/testify/assert" @@ -1058,8 +1059,10 @@ func TestGroupConcatWithAggrOnEngine(t *testing.T) { for _, tcase := range tcases { t.Run(tcase.name, func(t *testing.T) { fp := &fakePrimitive{results: []*sqltypes.Result{tcase.inputResult}} + agp := NewAggregateParam(AggregateGroupConcat, 1, "group_concat(c2)", collations.MySQL8()) + agp.Func = &sqlparser.GroupConcatExpr{} oa := &OrderedAggregate{ - Aggregates: []*AggregateParams{NewAggregateParam(AggregateGroupConcat, 1, "group_concat(c2)", collations.MySQL8())}, + Aggregates: []*AggregateParams{agp}, GroupByKeys: []*GroupByParams{{KeyCol: 0}}, Input: fp, } @@ -1137,8 +1140,10 @@ func TestGroupConcat(t *testing.T) { for _, tcase := range tcases { t.Run(tcase.name, func(t *testing.T) { fp := &fakePrimitive{results: []*sqltypes.Result{tcase.inputResult}} + agp := NewAggregateParam(AggregateGroupConcat, 1, "", collations.MySQL8()) + agp.Func = &sqlparser.GroupConcatExpr{} oa := &OrderedAggregate{ - Aggregates: []*AggregateParams{NewAggregateParam(AggregateGroupConcat, 1, "", collations.MySQL8())}, + Aggregates: []*AggregateParams{agp}, GroupByKeys: []*GroupByParams{{KeyCol: 0}}, Input: fp, } diff --git a/go/vt/vtgate/engine/scalar_aggregation_test.go b/go/vt/vtgate/engine/scalar_aggregation_test.go index 99031c95f34..613bb0c8555 100644 --- a/go/vt/vtgate/engine/scalar_aggregation_test.go +++ b/go/vt/vtgate/engine/scalar_aggregation_test.go @@ -27,6 +27,7 @@ import ( "vitess.io/vitess/go/mysql/collations" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/test/utils" + "vitess.io/vitess/go/vt/sqlparser" . "vitess.io/vitess/go/vt/vtgate/engine/opcode" ) @@ -233,6 +234,7 @@ func TestScalarGroupConcatWithAggrOnEngine(t *testing.T) { Opcode: AggregateGroupConcat, Col: 0, Alias: "group_concat(c2)", + Func: &sqlparser.GroupConcatExpr{}, }}, Input: fp, } @@ -394,6 +396,7 @@ func TestScalarGroupConcat(t *testing.T) { Aggregates: []*AggregateParams{{ Opcode: AggregateGroupConcat, Col: 0, + Func: &sqlparser.GroupConcatExpr{}, }}, Input: fp, } From 1d9c21a0037dd06a3dd0fc6e7b4663abfcd5a6ef Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Mon, 24 Jun 2024 11:06:41 +0530 Subject: [PATCH 5/7] feat: move checking of empty separator on plan time Signed-off-by: Manan Gupta --- go/vt/vtgate/engine/aggregations.go | 5 +---- go/vt/vtgate/engine/ordered_aggregate_test.go | 4 ++-- go/vt/vtgate/engine/scalar_aggregation_test.go | 4 ++-- go/vt/vtgate/planbuilder/operator_transformers.go | 3 +++ report.xml | 11 +++++++++++ 5 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 report.xml diff --git a/go/vt/vtgate/engine/aggregations.go b/go/vt/vtgate/engine/aggregations.go index 35df2704fea..d3f2b7a1c82 100644 --- a/go/vt/vtgate/engine/aggregations.go +++ b/go/vt/vtgate/engine/aggregations.go @@ -435,11 +435,8 @@ func newAggregation(fields []*querypb.Field, aggregates []*AggregateParams) (agg ag = &aggregatorScalar{from: aggr.Col} case AggregateGroupConcat: - separator := []byte{','} gcFunc := aggr.Func.(*sqlparser.GroupConcatExpr) - if gcFunc.Separator != "" { - separator = []byte(gcFunc.Separator) - } + separator := []byte(gcFunc.Separator) ag = &aggregatorGroupConcat{ from: aggr.Col, type_: targetType, diff --git a/go/vt/vtgate/engine/ordered_aggregate_test.go b/go/vt/vtgate/engine/ordered_aggregate_test.go index 13da9beaf6c..c601654bced 100644 --- a/go/vt/vtgate/engine/ordered_aggregate_test.go +++ b/go/vt/vtgate/engine/ordered_aggregate_test.go @@ -1060,7 +1060,7 @@ func TestGroupConcatWithAggrOnEngine(t *testing.T) { t.Run(tcase.name, func(t *testing.T) { fp := &fakePrimitive{results: []*sqltypes.Result{tcase.inputResult}} agp := NewAggregateParam(AggregateGroupConcat, 1, "group_concat(c2)", collations.MySQL8()) - agp.Func = &sqlparser.GroupConcatExpr{} + agp.Func = &sqlparser.GroupConcatExpr{Separator: ","} oa := &OrderedAggregate{ Aggregates: []*AggregateParams{agp}, GroupByKeys: []*GroupByParams{{KeyCol: 0}}, @@ -1141,7 +1141,7 @@ func TestGroupConcat(t *testing.T) { t.Run(tcase.name, func(t *testing.T) { fp := &fakePrimitive{results: []*sqltypes.Result{tcase.inputResult}} agp := NewAggregateParam(AggregateGroupConcat, 1, "", collations.MySQL8()) - agp.Func = &sqlparser.GroupConcatExpr{} + agp.Func = &sqlparser.GroupConcatExpr{Separator: ","} oa := &OrderedAggregate{ Aggregates: []*AggregateParams{agp}, GroupByKeys: []*GroupByParams{{KeyCol: 0}}, diff --git a/go/vt/vtgate/engine/scalar_aggregation_test.go b/go/vt/vtgate/engine/scalar_aggregation_test.go index 613bb0c8555..6fa0c8aecb8 100644 --- a/go/vt/vtgate/engine/scalar_aggregation_test.go +++ b/go/vt/vtgate/engine/scalar_aggregation_test.go @@ -234,7 +234,7 @@ func TestScalarGroupConcatWithAggrOnEngine(t *testing.T) { Opcode: AggregateGroupConcat, Col: 0, Alias: "group_concat(c2)", - Func: &sqlparser.GroupConcatExpr{}, + Func: &sqlparser.GroupConcatExpr{Separator: ","}, }}, Input: fp, } @@ -396,7 +396,7 @@ func TestScalarGroupConcat(t *testing.T) { Aggregates: []*AggregateParams{{ Opcode: AggregateGroupConcat, Col: 0, - Func: &sqlparser.GroupConcatExpr{}, + Func: &sqlparser.GroupConcatExpr{Separator: ","}, }}, Input: fp, } diff --git a/go/vt/vtgate/planbuilder/operator_transformers.go b/go/vt/vtgate/planbuilder/operator_transformers.go index 9d251fde8f2..08f9f694f0f 100644 --- a/go/vt/vtgate/planbuilder/operator_transformers.go +++ b/go/vt/vtgate/planbuilder/operator_transformers.go @@ -309,6 +309,9 @@ func transformAggregator(ctx *plancontext.PlanningContext, op *operators.Aggrega } aggrParam := engine.NewAggregateParam(aggr.OpCode, aggr.ColOffset, aggr.Alias, ctx.VSchema.Environment().CollationEnv()) aggrParam.Func = aggr.Func + if gcFunc, isGc := aggrParam.Func.(*sqlparser.GroupConcatExpr); isGc && gcFunc.Separator == "" { + gcFunc.Separator = "," + } aggrParam.Original = aggr.Original aggrParam.OrigOpcode = aggr.OriginalOpCode aggrParam.WCol = aggr.WSOffset diff --git a/report.xml b/report.xml new file mode 100644 index 00000000000..d3aef5acbd5 --- /dev/null +++ b/report.xml @@ -0,0 +1,11 @@ + + + + + + + + + + + From b0a75eacdb475c733e4196a6f18076aea82d4dba Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Mon, 24 Jun 2024 12:40:43 +0530 Subject: [PATCH 6/7] cleanup: remove report.xml Signed-off-by: Manan Gupta --- report.xml | 11 ----------- 1 file changed, 11 deletions(-) delete mode 100644 report.xml diff --git a/report.xml b/report.xml deleted file mode 100644 index d3aef5acbd5..00000000000 --- a/report.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - - - - - - - - From e6031625d7e2b865254693d71a98cce0636b551c Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Mon, 24 Jun 2024 12:43:55 +0530 Subject: [PATCH 7/7] feat: add a constant to store the group concat default separator Signed-off-by: Manan Gupta --- .gitignore | 2 ++ go/vt/sqlparser/constants.go | 3 +++ go/vt/vtgate/planbuilder/operator_transformers.go | 2 +- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 43f352d1b80..70ae13ad32d 100644 --- a/.gitignore +++ b/.gitignore @@ -56,6 +56,8 @@ __debug_bin /php/composer.phar /php/vendor +report*.xml + # vitess.io preview site /preview-vitess.io/ diff --git a/go/vt/sqlparser/constants.go b/go/vt/sqlparser/constants.go index 024a2148c33..228546cb422 100644 --- a/go/vt/sqlparser/constants.go +++ b/go/vt/sqlparser/constants.go @@ -478,6 +478,9 @@ const ( // KillType strings ConnectionStr = "connection" QueryStr = "query" + + // GroupConcatDefaultSeparator is the default separator for GroupConcatExpr. + GroupConcatDefaultSeparator = "," ) // Constants for Enum Type - Insert.Action diff --git a/go/vt/vtgate/planbuilder/operator_transformers.go b/go/vt/vtgate/planbuilder/operator_transformers.go index 08f9f694f0f..e89dfaaf848 100644 --- a/go/vt/vtgate/planbuilder/operator_transformers.go +++ b/go/vt/vtgate/planbuilder/operator_transformers.go @@ -310,7 +310,7 @@ func transformAggregator(ctx *plancontext.PlanningContext, op *operators.Aggrega aggrParam := engine.NewAggregateParam(aggr.OpCode, aggr.ColOffset, aggr.Alias, ctx.VSchema.Environment().CollationEnv()) aggrParam.Func = aggr.Func if gcFunc, isGc := aggrParam.Func.(*sqlparser.GroupConcatExpr); isGc && gcFunc.Separator == "" { - gcFunc.Separator = "," + gcFunc.Separator = sqlparser.GroupConcatDefaultSeparator } aggrParam.Original = aggr.Original aggrParam.OrigOpcode = aggr.OriginalOpCode