From db86a6f99bdbfb730c2c269eb1bbc72c15d71865 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Mon, 28 Oct 2024 12:20:51 +0100 Subject: [PATCH] More tests --- .../src/main/resources/mv_expand.csv-spec | 12 +- .../xpack/esql/action/EsqlCapabilities.java | 2 +- .../xpack/esql/parser/LogicalPlanBuilder.java | 2 +- .../xpack/esql/plan/logical/MvExpand.java | 9 +- .../optimizer/LogicalPlanOptimizerTests.java | 138 ++++++++++++++++++ .../esql/parser/StatementParserTests.java | 3 +- .../logical/MvExpandSerializationTests.java | 4 +- 7 files changed, 155 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec index 2448d01cbd897..2bf7951904bc3 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec @@ -328,7 +328,7 @@ avg_worked_seconds:long | birth_date:date | emp_no:integer | first_name // see https://github.com/elastic/elasticsearch/issues/102061 sortMvExpand -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = 1 | sort a | mv_expand a; a:integer @@ -338,7 +338,7 @@ a:integer // see https://github.com/elastic/elasticsearch/issues/102061 limitSortMvExpand -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = 1 | limit 1 | sort a | mv_expand a; a:integer @@ -348,7 +348,7 @@ a:integer // see https://github.com/elastic/elasticsearch/issues/102061 limitSortMultipleMvExpand -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = [1, 2, 3, 4, 5], b = 2, c = 3 | sort a | mv_expand a | mv_expand b | mv_expand c | limit 3; a:integer | b:integer | c:integer @@ -359,7 +359,7 @@ a:integer | b:integer | c:integer multipleLimitSortMultipleMvExpand -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = [1, 2, 3, 4, 5], b = 2, c = 3 | sort a | mv_expand a | limit 2 | mv_expand b | mv_expand c | limit 3; a:integer | b:integer | c:integer @@ -369,7 +369,7 @@ a:integer | b:integer | c:integer multipleLimitSortMultipleMvExpand2 -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = [1, 2, 3, 4, 5], b = 2, c = 3 | sort a | mv_expand a | limit 3 | mv_expand b | mv_expand c | limit 2; a:integer | b:integer | c:integer @@ -380,7 +380,7 @@ a:integer | b:integer | c:integer //see https://github.com/elastic/elasticsearch/issues/102084 whereMvExpand -required_capability: fix_mv_expand_limit_pushdown +required_capability: add_limit_inside_mv_expand row a = 1, b = -15 | where b > 3 | mv_expand b; a:integer | b:integer diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index 15f763e7f1047..6439df6ee71ee 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -446,7 +446,7 @@ public enum Cap { /** * Fix pushdown of LIMIT past MV_EXPAND */ - FIX_MV_EXPAND_LIMIT_PUSHDOWN; + ADD_LIMIT_INSIDE_MV_EXPAND; private final boolean enabled; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java index e4310d6aa9d91..dc913cd2f14f4 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java @@ -227,7 +227,7 @@ public PlanFactory visitDissectCommand(EsqlBaseParser.DissectCommandContext ctx) public PlanFactory visitMvExpandCommand(EsqlBaseParser.MvExpandCommandContext ctx) { UnresolvedAttribute field = visitQualifiedName(ctx.qualifiedName()); Source src = source(ctx); - return child -> new MvExpand(src, child, field, new UnresolvedAttribute(src, field.name()), null); + return child -> new MvExpand(src, child, field, new UnresolvedAttribute(src, field.name())); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java index dfeb01a33417f..949e4906e5033 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java @@ -31,6 +31,10 @@ public class MvExpand extends UnaryPlan { private List output; + public MvExpand(Source source, LogicalPlan child, NamedExpression target, Attribute expanded) { + this(source, child, target, expanded, null); + } + public MvExpand(Source source, LogicalPlan child, NamedExpression target, Attribute expanded, Integer limit) { super(source, child); this.target = target; @@ -128,8 +132,7 @@ public boolean equals(Object obj) { if (false == super.equals(obj)) { return false; } - return Objects.equals(target, ((MvExpand) obj).target) - && Objects.equals(expanded, ((MvExpand) obj).expanded) - && Objects.equals(limit, ((MvExpand) obj).limit); + MvExpand other = ((MvExpand) obj); + return Objects.equals(target, other.target) && Objects.equals(expanded, other.expanded) && Objects.equals(limit, other.limit); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index a9cc196ec34bd..228ff12d80973 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -1485,6 +1485,144 @@ public void testPushDownLimit_PastEvalAndMvExpand() { as(topN.child(), EsRelation.class); } + /** + * Expected + * EsqlProject[[emp_no{f}#12, first_name{r}#22, salary{f}#17]] + * \_TopN[[Order[salary{f}#17,ASC,LAST], Order[first_name{r}#22,ASC,LAST]],1000[INTEGER]] + * \_Filter[gender{f}#14 == [46][KEYWORD] AND WILDCARDLIKE(first_name{r}#22)] + * \_MvExpand[first_name{f}#13,first_name{r}#22,null] + * \_TopN[[Order[emp_no{f}#12,ASC,LAST]],10000[INTEGER]] + * \_EsRelation[test][_meta_field{f}#18, emp_no{f}#12, first_name{f}#13, ..] + */ + public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedField_ResultTruncationDefaultSize() { + LogicalPlan plan = optimizedPlan(""" + from test + | sort emp_no + | mv_expand first_name + | where gender == "F" + | where first_name LIKE "R*" + | keep emp_no, first_name, salary + | sort salary, first_name"""); + + var keep = as(plan, EsqlProject.class); + var topN = as(keep.child(), TopN.class); + assertThat(topN.limit().fold(), equalTo(1000)); + assertThat(orderNames(topN), contains("salary", "first_name")); + var filter = as(topN.child(), Filter.class); + assertThat(filter.condition(), instanceOf(And.class)); + var mvExp = as(filter.child(), MvExpand.class); + topN = as(mvExp.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule + assertThat(orderNames(topN), contains("emp_no")); + as(topN.child(), EsRelation.class); + } + + /** + * Expected + * + * MvExpand[first_name{f}#7,first_name{r}#16,10] + * \_TopN[[Order[emp_no{f}#6,DESC,FIRST]],10[INTEGER]] + * \_Filter[emp_no{f}#6 ≤ 10006[INTEGER]] + * \_EsRelation[test][_meta_field{f}#12, emp_no{f}#6, first_name{f}#7, ge..] + */ + public void testFilterWithSortBeforeMvExpand() { + LogicalPlan plan = optimizedPlan(""" + from test + | where emp_no <= 10006 + | sort emp_no desc + | mv_expand first_name + | limit 10"""); + + var mvExp = as(plan, MvExpand.class); + assertThat(mvExp.limit(), equalTo(10)); + var topN = as(mvExp.child(), TopN.class); + assertThat(topN.limit().fold(), equalTo(10)); + assertThat(orderNames(topN), contains("emp_no")); + var filter = as(topN.child(), Filter.class); + as(filter.child(), EsRelation.class); + } + + /** + * Expected + * + * TopN[[Order[first_name{f}#10,ASC,LAST]],500[INTEGER]] + * \_MvExpand[last_name{f}#13,last_name{r}#20,null] + * \_Filter[emp_no{r}#19 > 10050[INTEGER]] + * \_MvExpand[emp_no{f}#9,emp_no{r}#19,null] + * \_EsRelation[test][_meta_field{f}#15, emp_no{f}#9, first_name{f}#10, g..] + */ + public void testMultiMvExpand_SortDownBelow() { + LogicalPlan plan = optimizedPlan(""" + from test + | sort last_name ASC + | mv_expand emp_no + | where emp_no > 10050 + | mv_expand last_name + | sort first_name"""); + + var topN = as(plan, TopN.class); + assertThat(topN.limit().fold(), equalTo(1000)); + assertThat(orderNames(topN), contains("first_name")); + var mvExpand = as(topN.child(), MvExpand.class); + var filter = as(mvExpand.child(), Filter.class); + mvExpand = as(filter.child(), MvExpand.class); + var topN2 = as(mvExpand.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule + as(topN2.child(), EsRelation.class); + } + + /** + * Expected + * + * MvExpand[c{r}#7,c{r}#16,10000] + * \_EsqlProject[[c{r}#7, a{r}#3]] + * \_TopN[[Order[a{r}#3,ASC,FIRST]],7300[INTEGER]] + * \_MvExpand[b{r}#5,b{r}#15,7300] + * \_Limit[7300[INTEGER]] + * \_Row[[null[NULL] AS a, 123[INTEGER] AS b, 234[INTEGER] AS c]] + */ + public void testLimitThenSortBeforeMvExpand() { + LogicalPlan plan = optimizedPlan(""" + row a = null, b = 123, c = 234 + | mv_expand b + | limit 7300 + | keep c, a + | sort a NULLS FIRST + | mv_expand c"""); + + var mvExpand = as(plan, MvExpand.class); + assertThat(mvExpand.limit(), equalTo(10000)); + var project = as(mvExpand.child(), EsqlProject.class); + var topN = as(project.child(), TopN.class); + assertThat(topN.limit().fold(), equalTo(7300)); + assertThat(orderNames(topN), contains("a")); + mvExpand = as(topN.child(), MvExpand.class); + var limit = as(mvExpand.child(), Limit.class); + assertThat(limit.limit().fold(), equalTo(7300)); + as(limit.child(), Row.class); + } + + + /** + * Expected + * TopN[[Order[first_name{r}#16,ASC,LAST]],10000[INTEGER]] + * \_MvExpand[first_name{f}#7,first_name{r}#16] + * \_EsRelation[test][_meta_field{f}#12, emp_no{f}#6, first_name{f}#7, ge..] + */ + public void testRemoveUnusedSortBeforeMvExpand_DefaultLimit10000() { + LogicalPlan plan = optimizedPlan(""" + from test + | sort emp_no + | mv_expand first_name + | sort first_name + | limit 15000"""); + + var topN = as(plan, TopN.class); + assertThat(orderNames(topN), contains("first_name")); + assertThat(topN.limit().fold(), equalTo(10000)); + var mvExpand = as(topN.child(), MvExpand.class); + var topN2 = as(mvExpand.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule + as(topN2.child(), EsRelation.class); + } + /** * Expected * EsqlProject[[emp_no{f}#104, first_name{f}#105, salary{f}#106]] diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java index 3602b8468e8e0..97de0caa93b5c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java @@ -1642,8 +1642,7 @@ public void testParamForIdentifier() { List.of(new Order(EMPTY, attribute("f.5.*"), Order.OrderDirection.ASC, Order.NullsPosition.LAST)) ), attribute("f.6*"), - attribute("f.6*"), - null + attribute("f.6*") ), statement( """ diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/MvExpandSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/MvExpandSerializationTests.java index 7bdbebefb7790..fe8286db046bb 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/MvExpandSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/MvExpandSerializationTests.java @@ -22,7 +22,7 @@ protected MvExpand createTestInstance() { LogicalPlan child = randomChild(0); NamedExpression target = FieldAttributeTests.createFieldAttribute(0, false); Attribute expanded = ReferenceAttributeTests.randomReferenceAttribute(false); - return new MvExpand(source, child, target, expanded, null); + return new MvExpand(source, child, target, expanded); } @Override @@ -35,7 +35,7 @@ protected MvExpand mutateInstance(MvExpand instance) throws IOException { case 1 -> target = randomValueOtherThan(target, () -> FieldAttributeTests.createFieldAttribute(0, false)); case 2 -> expanded = randomValueOtherThan(expanded, () -> ReferenceAttributeTests.randomReferenceAttribute(false)); } - return new MvExpand(instance.source(), child, target, expanded, null); + return new MvExpand(instance.source(), child, target, expanded); } @Override