From d9d63a2d3a8ab10475f672374d04105de8b7cdaf Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Wed, 26 Jun 2024 15:49:53 +0200 Subject: [PATCH 1/6] Fix Incorrect Optimization with LIMIT and GROUP BY (#16263) Signed-off-by: Andres Taylor --- .../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 531e1077bf6..e8eba56bbcc 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" @@ -69,6 +70,18 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } } +func TestAggrWithLimit(t *testing.T) { + utils.SkipIfBinaryIsBelowVersion(t, 21, "vtgate") + mcmp, closer := start(t) + defer closer() + + for i := range 1000 { + 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 2bd40c2aaa5..8c946ecd41d 100644 --- a/go/vt/vtgate/planbuilder/operators/query_planning.go +++ b/go/vt/vtgate/planbuilder/operators/query_planning.go @@ -490,6 +490,11 @@ func setUpperLimit(in *Limit) (Operator, *ApplyResult) { case *Join, *ApplyJoin, *SubQueryContainer, *SubQuery: // we can't push limits down on either side return SkipChildren + case *Aggregator: + if len(op.Grouping) > 0 { + // we can't push limits down if we have a group by + return SkipChildren + } case *Route: newSrc := &Limit{ Source: op.Source, @@ -498,9 +503,8 @@ func setUpperLimit(in *Limit) (Operator, *ApplyResult) { op.Source = newSrc result = result.Merge(Rewrote("push upper limit under route")) return SkipChildren - default: - return VisitChildren } + return VisitChildren } 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 34f198abb96..3c93959a3ff 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 d01710f0e42b7652e80b80fdb25de6bc9a878bfb Mon Sep 17 00:00:00 2001 From: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Date: Wed, 26 Jun 2024 08:57:35 -0600 Subject: [PATCH 2/6] Update go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go Signed-off-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com> --- go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go | 2 +- 1 file changed, 1 insertion(+), 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 e8eba56bbcc..73403c82157 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -71,7 +71,7 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } func TestAggrWithLimit(t *testing.T) { - utils.SkipIfBinaryIsBelowVersion(t, 21, "vtgate") + utils.SkipIfBinaryIsBelowVersion(t, 20, "vtgate") mcmp, closer := start(t) defer closer() From d05f9827e3f3a95dda0451bfeb68bd1814fe97dc Mon Sep 17 00:00:00 2001 From: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Date: Wed, 26 Jun 2024 08:59:37 -0600 Subject: [PATCH 3/6] Update go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go Signed-off-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Signed-off-by: Florent Poinsard --- go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go | 1 - 1 file changed, 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 73403c82157..daa9b94a96c 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -71,7 +71,6 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } func TestAggrWithLimit(t *testing.T) { - utils.SkipIfBinaryIsBelowVersion(t, 20, "vtgate") mcmp, closer := start(t) defer closer() From 2421535100ceb6619467481bd0d01aebc24976b6 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 27 Jun 2024 08:47:11 +0200 Subject: [PATCH 4/6] test: make the test fail without fixes Signed-off-by: Andres Taylor --- go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go | 2 +- 1 file changed, 1 insertion(+), 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 daa9b94a96c..0de47ae0272 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -75,7 +75,7 @@ func TestAggrWithLimit(t *testing.T) { defer closer() for i := range 1000 { - 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 09e9234663367e4ab8c6d0699fbfdeff1aa7775e Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 27 Jun 2024 13:15:06 +0200 Subject: [PATCH 5/6] test: don't run on older versions Signed-off-by: Andres Taylor --- go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index 0de47ae0272..50d7e5be7a6 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -71,6 +71,7 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } func TestAggrWithLimit(t *testing.T) { + utils.SkipIfBinaryIsBelowVersion(t, 20, "vtgate") mcmp, closer := start(t) defer closer() From 6a3f0cd4d6e5035aefa6fd221c8a830f703b1826 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 27 Jun 2024 13:33:36 +0200 Subject: [PATCH 6/6] 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 50d7e5be7a6..e1f91ee2e08 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -71,7 +71,11 @@ func start(t *testing.T) (utils.MySQLCompare, func()) { } func TestAggrWithLimit(t *testing.T) { - utils.SkipIfBinaryIsBelowVersion(t, 20, "vtgate") + version, err := cluster.GetMajorVersion("vtgate") + require.NoError(t, err) + if version != 20 { + t.Skip("Test requires VTGate version 20") + } mcmp, closer := start(t) defer closer()