Skip to content

Commit 0ba7a4a

Browse files
committed
Further fixes and addressing reviews
1 parent f136b79 commit 0ba7a4a

File tree

14 files changed

+122
-64
lines changed

14 files changed

+122
-64
lines changed

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
8484
import org.elasticsearch.xpack.esql.plan.logical.Limit;
8585
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
86+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
8687
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
8788
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
8889
import org.elasticsearch.xpack.esql.plugin.EsqlPlugin;
@@ -446,7 +447,7 @@ public static Literal L(Object value) {
446447
}
447448

448449
public static LogicalPlan emptySource() {
449-
return new LocalRelation(Source.EMPTY, emptyList(), LocalSupplier.EMPTY);
450+
return new LocalRelation(Source.EMPTY, emptyList(), EmptyLocalSupplier.EMPTY);
450451
}
451452

452453
public static LogicalPlan localSource(BlockFactory blockFactory, List<Attribute> fields, List<Object> row) {

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateEmptyRelation.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
2121
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
2222
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan;
23+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
2324
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
2425
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
2526
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
@@ -36,7 +37,7 @@ public PropagateEmptyRelation() {
3637
@Override
3738
protected LogicalPlan rule(UnaryPlan plan, LogicalOptimizerContext ctx) {
3839
LogicalPlan p = plan;
39-
if (plan.child() instanceof LocalRelation local && local.supplier() == LocalSupplier.EMPTY) {
40+
if (plan.child() instanceof LocalRelation local && local.supplier() == EmptyLocalSupplier.EMPTY) {
4041
// only care about non-grouped aggs might return something (count)
4142
if (plan instanceof Aggregate agg && agg.groupings().isEmpty()) {
4243
List<Block> emptyBlocks = aggsFromEmpty(ctx.foldCtx(), agg.aggregates());

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumns.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ public LogicalPlan apply(LogicalPlan plan) {
6767
return p;
6868
}
6969

70+
// TODO: INLINESTATS unit testing for tracking this set
7071
if (p instanceof InlineJoin ij) {
7172
inlineJoinRightOutput.addAll(ij.right().outputSet());
7273
}
@@ -78,6 +79,7 @@ public LogicalPlan apply(LogicalPlan plan) {
7879
do {
7980
recheck = false;
8081
if (p instanceof Aggregate aggregate) {
82+
// TODO: INLINESTATS https://github.com/elastic/elasticsearch/pull/128917#discussion_r2175162099
8183
var remaining = removeUnused(aggregate.aggregates(), used, inlineJoinRightOutput);
8284

8385
if (remaining != null) {

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneEmptyPlans.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,13 @@
99

1010
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
1111
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan;
12+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
1213
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
13-
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
1414

1515
public final class PruneEmptyPlans extends OptimizerRules.OptimizerRule<UnaryPlan> {
1616

1717
public static LogicalPlan skipPlan(UnaryPlan plan) {
18-
return new LocalRelation(plan.source(), plan.output(), LocalSupplier.EMPTY);
18+
return new LocalRelation(plan.source(), plan.output(), EmptyLocalSupplier.EMPTY);
1919
}
2020

2121
@Override

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/SkipQueryOnEmptyMappings.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,13 @@
99

1010
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
1111
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
12+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
1213
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
13-
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
1414

1515
public final class SkipQueryOnEmptyMappings extends OptimizerRules.OptimizerRule<EsRelation> {
1616

1717
@Override
1818
protected LogicalPlan rule(EsRelation plan) {
19-
return plan.concreteIndices().isEmpty() ? new LocalRelation(plan.source(), plan.output(), LocalSupplier.EMPTY) : plan;
19+
return plan.concreteIndices().isEmpty() ? new LocalRelation(plan.source(), plan.output(), EmptyLocalSupplier.EMPTY) : plan;
2020
}
2121
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/PlanWritables.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin;
3030
import org.elasticsearch.xpack.esql.plan.logical.join.Join;
3131
import org.elasticsearch.xpack.esql.plan.logical.local.CopyingLocalSupplier;
32+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
3233
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject;
3334
import org.elasticsearch.xpack.esql.plan.logical.local.ImmediateLocalSupplier;
3435
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
@@ -129,6 +130,6 @@ public static List<NamedWriteableRegistry.Entry> physical() {
129130
}
130131

131132
public static List<NamedWriteableRegistry.Entry> others() {
132-
return List.of(CopyingLocalSupplier.ENTRY, ImmediateLocalSupplier.ENTRY);
133+
return List.of(CopyingLocalSupplier.ENTRY, ImmediateLocalSupplier.ENTRY, EmptyLocalSupplier.ENTRY);
133134
}
134135
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ public static LogicalPlan inlineData(InlineJoin target, LocalRelation data) {
7777

7878
/**
7979
* Replaces the stubbed source with the actual source.
80+
* NOTE: this will replace all {@link StubRelation}s found with the source and the method is meant to be used to replace one node only
81+
* when being called on a plan that has only ONE StubRelation in it.
8082
*/
8183
public static LogicalPlan replaceStub(LogicalPlan source, LogicalPlan stubbed) {
8284
// here we could have used stubbed.transformUp(StubRelation.class, stubRelation -> source)
@@ -129,7 +131,7 @@ public static LogicalPlanTuple firstSubPlan(LogicalPlan optimizedPlan) {
129131
optimizedPlan.forEachUp(InlineJoin.class, ij -> {
130132
// extract the right side of the plan and replace its source
131133
if (subPlan.get() == null && ij.right().anyMatch(p -> p instanceof StubRelation)) {
132-
var p = InlineJoin.replaceStub(ij.left(), ij.right());
134+
var p = replaceStub(ij.left(), ij.right());
133135
p.setOptimized();
134136
subPlan.set(new LogicalPlanTuple(p, ij.right()));
135137
}
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.plan.logical.local;
9+
10+
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
11+
import org.elasticsearch.common.io.stream.StreamOutput;
12+
import org.elasticsearch.common.io.stream.Writeable;
13+
import org.elasticsearch.compute.data.Block;
14+
import org.elasticsearch.compute.data.BlockUtils;
15+
16+
import java.io.IOException;
17+
18+
public class EmptyLocalSupplier implements LocalSupplier {
19+
20+
public static final LocalSupplier EMPTY = new EmptyLocalSupplier();
21+
public static final String NAME = "EmptySupplier";
22+
private static final Writeable.Reader<LocalSupplier> EMPTY_LOCAL_SUPPLIER_READER = in -> {
23+
assert in.readVInt() == 0;
24+
return EMPTY;
25+
};
26+
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
27+
LocalSupplier.class,
28+
NAME,
29+
EMPTY_LOCAL_SUPPLIER_READER
30+
);
31+
32+
private EmptyLocalSupplier() {}
33+
34+
@Override
35+
public String getWriteableName() {
36+
return NAME;
37+
}
38+
39+
@Override
40+
public Block[] get() {
41+
return BlockUtils.NO_BLOCKS;
42+
}
43+
44+
@Override
45+
public String toString() {
46+
return "EMPTY";
47+
}
48+
49+
@Override
50+
public void writeTo(StreamOutput out) throws IOException {
51+
out.writeVInt(0);
52+
}
53+
54+
@Override
55+
public boolean equals(Object obj) {
56+
return obj == EMPTY;
57+
}
58+
59+
@Override
60+
public int hashCode() {
61+
return 0;
62+
}
63+
64+
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/local/LocalSupplier.java

Lines changed: 2 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,8 @@
88
package org.elasticsearch.xpack.esql.plan.logical.local;
99

1010
import org.elasticsearch.common.io.stream.NamedWriteable;
11-
import org.elasticsearch.common.io.stream.StreamOutput;
1211
import org.elasticsearch.common.io.stream.Writeable;
1312
import org.elasticsearch.compute.data.Block;
14-
import org.elasticsearch.compute.data.BlockUtils;
1513
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
1614

1715
import java.io.IOException;
@@ -26,46 +24,14 @@
2624
* {@link UnsupportedOperationException}.
2725
* </p>
2826
*/
29-
public interface LocalSupplier extends Supplier<Block[]>, Writeable, NamedWriteable {
30-
31-
LocalSupplier EMPTY = new LocalSupplier() {
32-
@Override
33-
public String getWriteableName() {
34-
return "EmptySupplier";
35-
}
36-
37-
@Override
38-
public Block[] get() {
39-
return BlockUtils.NO_BLOCKS;
40-
}
41-
42-
@Override
43-
public String toString() {
44-
return "EMPTY";
45-
}
46-
47-
@Override
48-
public void writeTo(StreamOutput out) throws IOException {
49-
out.writeVInt(0);
50-
}
51-
52-
@Override
53-
public boolean equals(Object obj) {
54-
return obj == EMPTY;
55-
}
56-
57-
@Override
58-
public int hashCode() {
59-
return 0;
60-
}
61-
};
27+
public interface LocalSupplier extends Supplier<Block[]>, NamedWriteable {
6228

6329
static LocalSupplier of(Block[] blocks) {
6430
return new ImmediateLocalSupplier(blocks);
6531
}
6632

6733
static LocalSupplier readFrom(PlanStreamInput in) throws IOException {
6834
Block[] blocks = in.readCachedBlockArray();
69-
return blocks.length == 0 ? EMPTY : of(blocks);
35+
return blocks.length == 0 ? EmptyLocalSupplier.EMPTY : of(blocks);
7036
}
7137
}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,9 @@
5252
import org.elasticsearch.xpack.esql.plan.logical.Project;
5353
import org.elasticsearch.xpack.esql.plan.logical.Row;
5454
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan;
55+
import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier;
5556
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject;
5657
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
57-
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
5858
import org.elasticsearch.xpack.esql.stats.SearchStats;
5959
import org.hamcrest.Matchers;
6060
import org.junit.BeforeClass;
@@ -681,7 +681,7 @@ public void testReplaceStringCasingAndRLikeWithLocalRelation() {
681681
var plan = localPlan("FROM test | WHERE TO_LOWER(TO_UPPER(first_name)) RLIKE \"VALÜ*\"");
682682

683683
var local = as(plan, LocalRelation.class);
684-
assertThat(local.supplier(), equalTo(LocalSupplier.EMPTY));
684+
assertThat(local.supplier(), equalTo(EmptyLocalSupplier.EMPTY));
685685
}
686686

687687
// same plan as in testReplaceUpperStringCasingWithInsensitiveRLike, but with LIKE instead of RLIKE
@@ -720,7 +720,7 @@ public void testReplaceStringCasingAndLikeWithLocalRelation() {
720720
var plan = localPlan("FROM test | WHERE TO_LOWER(TO_UPPER(first_name)) LIKE \"VALÜ*\"");
721721

722722
var local = as(plan, LocalRelation.class);
723-
assertThat(local.supplier(), equalTo(LocalSupplier.EMPTY));
723+
assertThat(local.supplier(), equalTo(EmptyLocalSupplier.EMPTY));
724724
}
725725

726726
/**
@@ -780,7 +780,7 @@ private IsNotNull isNotNull(Expression field) {
780780

781781
private LocalRelation asEmptyRelation(Object o) {
782782
var empty = as(o, LocalRelation.class);
783-
assertThat(empty.supplier(), is(LocalSupplier.EMPTY));
783+
assertThat(empty.supplier(), is(EmptyLocalSupplier.EMPTY));
784784
return empty;
785785
}
786786

0 commit comments

Comments
 (0)