Skip to content

Commit 9bd4b89

Browse files
nik9000nik9000
committed
Proper delegate
1 parent f5ce702 commit 9bd4b89

File tree

8 files changed

+139
-50
lines changed

8 files changed

+139
-50
lines changed

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/Equals.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
2424
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.LucenePushdownPredicates;
2525
import org.elasticsearch.xpack.esql.planner.TranslatorHandler;
26+
import org.elasticsearch.xpack.esql.querydsl.query.EqualsSyntheticSourceDelegate;
27+
import org.elasticsearch.xpack.esql.querydsl.query.SingleValueQuery;
2628

2729
import java.time.ZoneId;
2830
import java.util.Map;
@@ -143,7 +145,8 @@ public Query asQuery(LucenePushdownPredicates pushdownPredicates, TranslatorHand
143145
if (left().dataType() == DataType.TEXT && left() instanceof FieldAttribute fa) {
144146
String value = ((BytesRef) lit.value()).utf8ToString();
145147
if (pushdownPredicates.canUseEqualityOnSyntheticSourceDelegate(fa, value)) {
146-
return new EqualsSyntheticSourceDelegate(source(), handler.nameOf(fa), value);
148+
String name = handler.nameOf(fa);
149+
return new SingleValueQuery(new EqualsSyntheticSourceDelegate(source(), name, value), name, true);
147150
}
148151
}
149152
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/TranslatorHandler.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,13 @@ public Query asQuery(LucenePushdownPredicates predicates, Expression e) {
4040
}
4141

4242
private static Query wrapFunctionQuery(Expression field, Query query) {
43+
if (query instanceof SingleValueQuery) {
44+
// Already wrapped
45+
return query;
46+
}
4347
if (field instanceof FieldAttribute fa) {
4448
fa = fa.getExactInfo().hasExact() ? fa.exactAttribute() : fa;
45-
return new SingleValueQuery(query, fa.name());
49+
return new SingleValueQuery(query, fa.name(), false);
4650
}
4751
if (field instanceof MetadataAttribute) {
4852
return query; // MetadataAttributes are always single valued

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/EqualsSyntheticSourceDelegate.java‎ renamed to ‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/querydsl/query/EqualsSyntheticSourceDelegate.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* 2.0.
66
*/
77

8-
package org.elasticsearch.xpack.esql.expression.predicate.operator.comparison;
8+
package org.elasticsearch.xpack.esql.querydsl.query;
99

1010
import org.elasticsearch.TransportVersion;
1111
import org.elasticsearch.index.mapper.TextFieldMapper;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueQuery.java‎

Lines changed: 112 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.elasticsearch.compute.operator.Warnings;
2121
import org.elasticsearch.compute.querydsl.query.SingleValueMatchQuery;
2222
import org.elasticsearch.index.mapper.MappedFieldType;
23+
import org.elasticsearch.index.mapper.TextFieldMapper;
2324
import org.elasticsearch.index.query.AbstractQueryBuilder;
2425
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
2526
import org.elasticsearch.index.query.QueryBuilder;
@@ -57,16 +58,28 @@ public class SingleValueQuery extends Query {
5758

5859
private final Query next;
5960
private final String field;
61+
private final boolean useSyntheticSourceDelegate;
6062

61-
public SingleValueQuery(Query next, String field) {
63+
/**
64+
* Build.
65+
* @param next the query whose documents we should use for single-valued fields
66+
* @param field the name of the field whose values to check
67+
* @param useSyntheticSourceDelegate Should we check the field's synthetic source delegate (true)
68+
* or it's values itself? If the field is a {@code text} field
69+
* we often want to use its delegate.
70+
*/
71+
public SingleValueQuery(Query next, String field, boolean useSyntheticSourceDelegate) {
6272
super(next.source());
6373
this.next = next;
6474
this.field = field;
75+
this.useSyntheticSourceDelegate = useSyntheticSourceDelegate;
6576
}
6677

6778
@Override
68-
protected Builder asBuilder() {
69-
return new Builder(next.toQueryBuilder(), field, next.source());
79+
protected AbstractBuilder asBuilder() {
80+
return useSyntheticSourceDelegate
81+
? new SyntheticSourceDelegateBuilder(next.toQueryBuilder(), field, next.source())
82+
: new Builder(next.toQueryBuilder(), field, next.source());
7083
}
7184

7285
@Override
@@ -76,7 +89,7 @@ protected String innerToString() {
7689

7790
@Override
7891
public SingleValueQuery negate(Source source) {
79-
return new SingleValueQuery(next.negate(source), field);
92+
return new SingleValueQuery(next.negate(source), field, useSyntheticSourceDelegate);
8093
}
8194

8295
@Override
@@ -85,26 +98,28 @@ public boolean equals(Object o) {
8598
return false;
8699
}
87100
SingleValueQuery other = (SingleValueQuery) o;
88-
return Objects.equals(next, other.next) && Objects.equals(field, other.field);
101+
return Objects.equals(next, other.next)
102+
&& Objects.equals(field, other.field)
103+
&& useSyntheticSourceDelegate == other.useSyntheticSourceDelegate;
89104
}
90105

91106
@Override
92107
public int hashCode() {
93-
return Objects.hash(super.hashCode(), next, field);
108+
return Objects.hash(super.hashCode(), next, field, useSyntheticSourceDelegate);
94109
}
95110

96-
public static class Builder extends AbstractQueryBuilder<Builder> {
111+
public abstract static class AbstractBuilder extends AbstractQueryBuilder<AbstractBuilder> {
97112
private final QueryBuilder next;
98113
private final String field;
99114
private final Source source;
100115

101-
Builder(QueryBuilder next, String field, Source source) {
116+
AbstractBuilder(QueryBuilder next, String field, Source source) {
102117
this.next = next;
103118
this.field = field;
104119
this.source = source;
105120
}
106121

107-
Builder(StreamInput in) throws IOException {
122+
AbstractBuilder(StreamInput in) throws IOException {
108123
super(in);
109124
this.next = in.readNamedWriteable(QueryBuilder.class);
110125
this.field = in.readString();
@@ -126,7 +141,7 @@ public static class Builder extends AbstractQueryBuilder<Builder> {
126141
}
127142

128143
@Override
129-
protected void doWriteTo(StreamOutput out) throws IOException {
144+
protected final void doWriteTo(StreamOutput out) throws IOException {
130145
out.writeNamedWriteable(next);
131146
out.writeString(field);
132147
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_16_0)) {
@@ -148,28 +163,11 @@ public Source source() {
148163
return source;
149164
}
150165

151-
@Override
152-
public String getWriteableName() {
153-
return ENTRY.name;
154-
}
155-
156-
@Override
157-
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
158-
builder.startObject(ENTRY.name);
159-
builder.field("field", field);
160-
builder.field("next", next, params);
161-
builder.field("source", source.toString());
162-
builder.endObject();
163-
}
164-
165-
@Override
166-
public TransportVersion getMinimalSupportedVersion() {
167-
return TransportVersions.V_8_11_X; // the first version of ESQL
168-
}
166+
protected abstract MappedFieldType mappedFieldType(SearchExecutionContext context);
169167

170168
@Override
171-
protected org.apache.lucene.search.Query doToQuery(SearchExecutionContext context) throws IOException {
172-
MappedFieldType ft = context.getFieldType(field);
169+
protected final org.apache.lucene.search.Query doToQuery(SearchExecutionContext context) throws IOException {
170+
MappedFieldType ft = mappedFieldType(context);
173171
if (ft == null) {
174172
return new MatchNoDocsQuery("missing field [" + field + "]");
175173
}
@@ -194,29 +192,109 @@ protected org.apache.lucene.search.Query doToQuery(SearchExecutionContext contex
194192
return builder.build();
195193
}
196194

195+
protected abstract AbstractBuilder rewrite(QueryBuilder next);
196+
197197
@Override
198-
protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
198+
protected final QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
199199
QueryBuilder rewritten = next.rewrite(queryRewriteContext);
200200
if (rewritten instanceof MatchNoneQueryBuilder) {
201201
return rewritten;
202202
}
203203
if (rewritten == next) {
204204
return this;
205205
}
206-
return new Builder(rewritten, field, source);
206+
return rewrite(rewritten);
207207
}
208208

209209
@Override
210-
protected boolean doEquals(Builder other) {
210+
protected final boolean doEquals(AbstractBuilder other) {
211211
return next.equals(other.next) && field.equals(other.field);
212212
}
213213

214214
@Override
215-
protected int doHashCode() {
215+
protected final int doHashCode() {
216216
return Objects.hash(next, field);
217217
}
218218
}
219219

220+
public static class Builder extends AbstractBuilder {
221+
Builder(QueryBuilder next, String field, Source source) {
222+
super(next, field, source);
223+
}
224+
225+
Builder(StreamInput in) throws IOException {
226+
super(in);
227+
}
228+
229+
@Override
230+
public String getWriteableName() {
231+
return ENTRY.name;
232+
}
233+
234+
@Override
235+
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
236+
builder.startObject(ENTRY.name);
237+
builder.field("field", field());
238+
builder.field("next", next(), params);
239+
builder.field("source", source().toString());
240+
builder.endObject();
241+
}
242+
243+
@Override
244+
public TransportVersion getMinimalSupportedVersion() {
245+
return TransportVersions.V_8_11_X; // the first version of ESQL
246+
}
247+
248+
@Override
249+
protected MappedFieldType mappedFieldType(SearchExecutionContext context) {
250+
return context.getFieldType(field());
251+
}
252+
253+
@Override
254+
protected AbstractBuilder rewrite(QueryBuilder next) {
255+
return new Builder(next, field(), source());
256+
}
257+
}
258+
259+
public static class SyntheticSourceDelegateBuilder extends AbstractBuilder {
260+
SyntheticSourceDelegateBuilder(QueryBuilder next, String field, Source source) {
261+
super(next, field, source);
262+
}
263+
264+
SyntheticSourceDelegateBuilder(StreamInput in) throws IOException {
265+
super(in);
266+
}
267+
268+
@Override
269+
public String getWriteableName() {
270+
throw new UnsupportedOperationException();
271+
}
272+
273+
@Override
274+
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
275+
builder.startObject(ENTRY.name);
276+
builder.field("field", field() + ":synthetic_source_delegate");
277+
builder.field("next", next(), params);
278+
builder.field("source", source().toString());
279+
builder.endObject();
280+
}
281+
282+
@Override
283+
public TransportVersion getMinimalSupportedVersion() {
284+
throw new UnsupportedOperationException();
285+
}
286+
287+
@Override
288+
protected MappedFieldType mappedFieldType(SearchExecutionContext context) {
289+
return ((TextFieldMapper.TextFieldType) context.getFieldType(field())).syntheticSourceDelegate();
290+
}
291+
292+
@Override
293+
protected AbstractBuilder rewrite(QueryBuilder next) {
294+
return new Builder(next, field(), source());
295+
}
296+
}
297+
220298
/**
221299
* Write a {@link Source} including the text in it.
222300
*/

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InTests.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.elasticsearch.xpack.esql.core.type.EsField;
2323
import org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase;
2424
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
25+
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.LucenePushdownPredicates;
2526
import org.elasticsearch.xpack.esql.planner.TranslatorHandler;
2627
import org.junit.AfterClass;
2728

@@ -93,7 +94,7 @@ public void testConvertedNull() {
9394
new FieldAttribute(Source.EMPTY, "field", new EsField("suffix", DataType.KEYWORD, Map.of(), true)),
9495
Arrays.asList(ONE, new Literal(Source.EMPTY, null, randomFrom(DataType.types())), THREE)
9596
);
96-
var query = in.asQuery(TranslatorHandler.TRANSLATOR_HANDLER);
97+
var query = in.asQuery(LucenePushdownPredicates.DEFAULT, TranslatorHandler.TRANSLATOR_HANDLER);
9798
assertEquals(new TermsQuery(EMPTY, "field", Set.of(1, 3)), query);
9899
}
99100

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@
8585
import org.elasticsearch.xpack.esql.expression.predicate.logical.Not;
8686
import org.elasticsearch.xpack.esql.expression.predicate.logical.Or;
8787
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals;
88-
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.EqualsSyntheticSourceDelegate;
8988
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.EsqlBinaryComparison;
9089
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThan;
9190
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThanOrEqual;
@@ -135,6 +134,7 @@
135134
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
136135
import org.elasticsearch.xpack.esql.planner.mapper.Mapper;
137136
import org.elasticsearch.xpack.esql.plugin.QueryPragmas;
137+
import org.elasticsearch.xpack.esql.querydsl.query.EqualsSyntheticSourceDelegate;
138138
import org.elasticsearch.xpack.esql.querydsl.query.SingleValueQuery;
139139
import org.elasticsearch.xpack.esql.querydsl.query.SpatialRelatesQuery;
140140
import org.elasticsearch.xpack.esql.session.Configuration;
@@ -7802,12 +7802,10 @@ public void testEqualsPushdownToDelegate() {
78027802
var project = as(exchange.child(), ProjectExec.class);
78037803
var extract = as(project.child(), FieldExtractExec.class);
78047804
var query = as(extract.child(), EsQueryExec.class);
7805-
// NOCOMMIT the single value query should target the synthetic source delegate.
78067805
assertThat(
78077806
query.query(),
7808-
equalTo(new SingleValueQuery(new EqualsSyntheticSourceDelegate(Source.EMPTY, "job", "v"), "job.raw").toQueryBuilder())
7807+
equalTo(new SingleValueQuery(new EqualsSyntheticSourceDelegate(Source.EMPTY, "job", "v"), "job", true).toQueryBuilder())
78097808
);
7810-
78117809
}
78127810

78137811
public void testEqualsPushdownToDelegateTooBig() {

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueQueryNegateTests.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,16 @@
2121
*/
2222
public class SingleValueQueryNegateTests extends ESTestCase {
2323
public void testNot() {
24-
var sv = new SingleValueQuery(new MatchAll(Source.EMPTY), "foo");
25-
assertThat(sv.negate(Source.EMPTY), equalTo(new SingleValueQuery(new NotQuery(Source.EMPTY, new MatchAll(Source.EMPTY)), "foo")));
24+
boolean useSyntheticSourceDelegate = randomBoolean();
25+
var sv = new SingleValueQuery(new MatchAll(Source.EMPTY), "foo", useSyntheticSourceDelegate);
26+
assertThat(
27+
sv.negate(Source.EMPTY),
28+
equalTo(new SingleValueQuery(new NotQuery(Source.EMPTY, new MatchAll(Source.EMPTY)), "foo", useSyntheticSourceDelegate))
29+
);
2630
}
2731

2832
public void testNotNot() {
29-
var sv = new SingleValueQuery(new MatchAll(Source.EMPTY), "foo");
33+
var sv = new SingleValueQuery(new MatchAll(Source.EMPTY), "foo", randomBoolean());
3034
assertThat(sv.negate(Source.EMPTY).negate(Source.EMPTY), equalTo(sv));
3135
}
3236
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueQueryTests.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public SingleValueQueryTests(Setup setup) {
6969
}
7070

7171
public void testMatchAll() throws IOException {
72-
testCase(new SingleValueQuery(new MatchAll(Source.EMPTY), "foo").asBuilder(), this::runCase);
72+
testCase(new SingleValueQuery(new MatchAll(Source.EMPTY), "foo", false).asBuilder(), this::runCase);
7373
}
7474

7575
public void testMatchSome() throws IOException {
@@ -100,22 +100,23 @@ public void testRewritesToMatchNone() throws IOException {
100100

101101
public void testNotMatchAll() throws IOException {
102102
testCase(
103-
new SingleValueQuery(new MatchAll(Source.EMPTY), "foo").negate(Source.EMPTY).asBuilder(),
103+
new SingleValueQuery(new MatchAll(Source.EMPTY), "foo", false).negate(Source.EMPTY).asBuilder(),
104104
(fieldValues, count) -> assertThat(count, equalTo(0))
105105
);
106106
}
107107

108108
public void testNotMatchNone() throws IOException {
109109
testCase(
110-
new SingleValueQuery(new MatchAll(Source.EMPTY).negate(Source.EMPTY), "foo").negate(Source.EMPTY).asBuilder(),
110+
new SingleValueQuery(new MatchAll(Source.EMPTY).negate(Source.EMPTY), "foo", false).negate(Source.EMPTY).asBuilder(),
111111
this::runCase
112112
);
113113
}
114114

115115
public void testNotMatchSome() throws IOException {
116116
int max = between(1, 100);
117117
testCase(
118-
new SingleValueQuery(new RangeQuery(Source.EMPTY, "i", null, false, max, false, null), "foo").negate(Source.EMPTY).asBuilder(),
118+
new SingleValueQuery(new RangeQuery(Source.EMPTY, "i", null, false, max, false, null), "foo", false).negate(Source.EMPTY)
119+
.asBuilder(),
119120
(fieldValues, count) -> runCase(fieldValues, count, max, 100)
120121
);
121122
}
@@ -161,7 +162,7 @@ private void runCase(List<List<Object>> fieldValues, int count) {
161162
runCase(fieldValues, count, null, null);
162163
}
163164

164-
private void testCase(SingleValueQuery.Builder builder, TestCase testCase) throws IOException {
165+
private void testCase(SingleValueQuery.AbstractBuilder builder, TestCase testCase) throws IOException {
165166
MapperService mapper = createMapperService(mapping(setup::mapping));
166167
try (Directory d = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), d)) {
167168
List<List<Object>> fieldValues = setup.build(iw);

0 commit comments

Comments
 (0)