From d667eed89dac2a12dd57123cbcda27bee259270e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Taylor?= Date: Wed, 26 Jun 2024 15:49:53 +0200 Subject: [PATCH 1/3] Fix Incorrect Optimization with LIMIT and GROUP BY (#16263) Signed-off-by: Andres Taylor Signed-off-by: Florent Poinsard --- .../vtgate/queries/aggregation/aggregation_test.go | 13 +++++++++++++ .../vtgate/planbuilder/operators/query_planning.go | 8 ++++++-- .../planbuilder/testdata/memory_sort_cases.json | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index 6af62bd557b..91eb1fb340a 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -18,6 +18,7 @@ package aggregation import ( "fmt" + "math/rand/v2" "slices" "sort" "strings" @@ -66,6 +67,18 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } } +func TestAggrWithLimit(t *testing.T) { + utils.SkipIfBinaryIsBelowVersion(t, 18, "vtgate") + mcmp, closer := start(t) + defer closer() + + for i := 0; i < 1000; i++ { + r := rand.IntN(10) + mcmp.Exec(fmt.Sprintf("insert into aggr_test(id, val1, val2) values(%d, 'a', %d)", i, r)) + } + mcmp.Exec("select val2, count(*) from aggr_test group by val2 order by count(*), val2 limit 10") +} + func TestAggregateTypes(t *testing.T) { mcmp, closer := start(t) defer closer() diff --git a/go/vt/vtgate/planbuilder/operators/query_planning.go b/go/vt/vtgate/planbuilder/operators/query_planning.go index 7d539b5c056..0af4270d71b 100644 --- a/go/vt/vtgate/planbuilder/operators/query_planning.go +++ b/go/vt/vtgate/planbuilder/operators/query_planning.go @@ -518,6 +518,11 @@ func setUpperLimit(in *Limit) (ops.Operator, *rewrite.ApplyResult, error) { case *Join, *ApplyJoin, *SubQueryContainer, *SubQuery: // we can't push limits down on either side return rewrite.SkipChildren + case *Aggregator: + if len(op.Grouping) > 0 { + // we can't push limits down if we have a group by + return rewrite.SkipChildren + } case *Route: newSrc := &Limit{ Source: op.Source, @@ -527,9 +532,8 @@ func setUpperLimit(in *Limit) (ops.Operator, *rewrite.ApplyResult, error) { op.Source = newSrc result = result.Merge(rewrite.NewTree("push limit under route", newSrc)) return rewrite.SkipChildren - default: - return rewrite.VisitChildren } + return rewrite.VisitChildren } _, err := rewrite.TopDown(in.Source, TableID, visitor, shouldVisit) diff --git a/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.json b/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.json index 3ad80eff59f..26d41896ea0 100644 --- a/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/memory_sort_cases.json @@ -147,7 +147,7 @@ }, "FieldQuery": "select a, b, count(*) as k, weight_string(a) from `user` where 1 != 1 group by a, weight_string(a)", "OrderBy": "(0|3) ASC", - "Query": "select a, b, count(*) as k, weight_string(a) from `user` group by a, weight_string(a) order by a asc limit :__upper_limit", + "Query": "select a, b, count(*) as k, weight_string(a) from `user` group by a, weight_string(a) order by a asc", "Table": "`user`" } ] From 37c8f8f35e3d67810450c43d457ff98bb64269f5 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 27 Jun 2024 08:47:11 +0200 Subject: [PATCH 2/3] test: make the test fail without fixes Signed-off-by: Andres Taylor --- .../endtoend/vtgate/queries/aggregation/aggregation_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index 91eb1fb340a..ed614ba8e32 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -18,7 +18,7 @@ package aggregation import ( "fmt" - "math/rand/v2" + "math/rand" "slices" "sort" "strings" @@ -73,7 +73,7 @@ func TestAggrWithLimit(t *testing.T) { defer closer() for i := 0; i < 1000; i++ { - r := rand.IntN(10) + r := rand.Intn(50) mcmp.Exec(fmt.Sprintf("insert into aggr_test(id, val1, val2) values(%d, 'a', %d)", i, r)) } mcmp.Exec("select val2, count(*) from aggr_test group by val2 order by count(*), val2 limit 10") From 09738c37fb22ca0172925bb68760eeea47714844 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 27 Jun 2024 13:33:36 +0200 Subject: [PATCH 3/3] test: Only run on this particular version Signed-off-by: Andres Taylor --- .../endtoend/vtgate/queries/aggregation/aggregation_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index ed614ba8e32..571936b2666 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -68,7 +68,11 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } func TestAggrWithLimit(t *testing.T) { - utils.SkipIfBinaryIsBelowVersion(t, 18, "vtgate") + version, err := cluster.GetMajorVersion("vtgate") + require.NoError(t, err) + if version != 18 { + t.Skip("Test requires VTGate version 18") + } mcmp, closer := start(t) defer closer()