From 6281905824a85facb5a6a281f10d4bda610748da Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Thu, 27 Jun 2024 17:44:54 +0200 Subject: [PATCH] [release-18.0] Fix Incorrect Optimization with LIMIT and GROUP BY (#16263) (#16266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Andres Taylor Signed-off-by: Florent Poinsard Co-authored-by: Andrés Taylor --- .../queries/aggregation/aggregation_test.go | 17 +++++++++++++++++ .../planbuilder/operators/query_planning.go | 8 ++++++-- .../planbuilder/testdata/memory_sort_cases.json | 2 +- 3 files changed, 24 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..571936b2666 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" "slices" "sort" "strings" @@ -66,6 +67,22 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } } +func TestAggrWithLimit(t *testing.T) { + 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() + + for i := 0; i < 1000; i++ { + 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") +} + 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`" } ]