Skip to content

Commit ab9bfdf

Browse files
GalLaloucheGalLalouche
committed
More review fixes
1 parent c0fe85e commit ab9bfdf

File tree

17 files changed

+215
-99
lines changed

17 files changed

+215
-99
lines changed

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/InvalidMappedField.java‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public InvalidMappedField(String name, String errorMessage) {
4545
* Constructor supporting union types, used in ES|QL.
4646
*/
4747
public InvalidMappedField(String name, Map<String, Set<String>> typesToIndices) {
48-
this(name, makeErrorMessage(typesToIndices), new TreeMap<>(), typesToIndices);
48+
this(name, makeErrorMessage(typesToIndices, false), new TreeMap<>(), typesToIndices);
4949
}
5050

5151
private InvalidMappedField(String name, String errorMessage, Map<String, EsField> properties, Map<String, Set<String>> typesToIndices) {
@@ -107,12 +107,21 @@ public Map<String, Set<String>> getTypesToIndices() {
107107
return typesToIndices;
108108
}
109109

110-
private static String makeErrorMessage(Map<String, Set<String>> typesToIndices) {
110+
public static String makeErrorsMessageIncludingInsistKeyword(Map<String, Set<String>> typesToIndices) {
111+
return makeErrorMessage(typesToIndices, true);
112+
}
113+
114+
private static String makeErrorMessage(Map<String, Set<String>> typesToIndices, boolean includeInsistKeyword) {
111115
StringBuilder errorMessage = new StringBuilder();
116+
var isInsistKeywordOnlyKeyword = includeInsistKeyword && typesToIndices.containsKey(DataType.KEYWORD.typeName()) == false;
112117
errorMessage.append("mapped as [");
113-
errorMessage.append(typesToIndices.size());
118+
errorMessage.append(typesToIndices.size() + (isInsistKeywordOnlyKeyword ? 1 : 0));
114119
errorMessage.append("] incompatible types: ");
115120
boolean first = true;
121+
if (isInsistKeywordOnlyKeyword) {
122+
first = false;
123+
errorMessage.append("[keyword] enforced by INSIST command");
124+
}
116125
for (Map.Entry<String, Set<String>> e : typesToIndices.entrySet()) {
117126
if (first) {
118127
first = false;
@@ -121,7 +130,12 @@ private static String makeErrorMessage(Map<String, Set<String>> typesToIndices)
121130
}
122131
errorMessage.append("[");
123132
errorMessage.append(e.getKey());
124-
errorMessage.append("] in ");
133+
errorMessage.append("] ");
134+
if (e.getKey().equals(DataType.KEYWORD.typeName()) && includeInsistKeyword) {
135+
errorMessage.append("enforced by INSIST command and in ");
136+
} else {
137+
errorMessage.append("in ");
138+
}
125139
if (e.getValue().size() <= 3) {
126140
errorMessage.append(e.getValue());
127141
} else {

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/PotentiallyUnmappedKeywordEsField.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@
1010

1111
import java.io.IOException;
1212

13-
/** Information about a field in an ES index that may not mapped. If it is unmapped, it should be loaded from source directly. */
13+
/**
14+
* This class is used as a marker for fields that may be unmapped, where an unmapped field is a field which exists in the _source but is not
15+
* mapped in the index. Note that this field may be mapped for some indices, but is unmapped in at least one of them.
16+
* For indices where the field is unmapped, we will try to load them directly from _source.
17+
*/
1418
public class PotentiallyUnmappedKeywordEsField extends KeywordEsField {
1519
public PotentiallyUnmappedKeywordEsField(String name) {
1620
super(name);

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,13 @@ public void close() {
230230
lineNumber++;
231231
}
232232
}
233-
var pageColumns = new ArrayList<String>(columns.length);
233+
var columnNames = new ArrayList<String>(columns.length);
234234
try {
235235
var blocks = Arrays.stream(columns)
236-
.peek(b -> pageColumns.add(b.name))
236+
.peek(b -> columnNames.add(b.name))
237237
.map(b -> b.builderWrapper.builder().build())
238238
.toArray(Block[]::new);
239-
return new Tuple<>(new Page(blocks), pageColumns);
239+
return new Tuple<>(new Page(blocks), columnNames);
240240
} finally {
241241
Releasables.closeExpectNoException(columns);
242242
}

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped_fields.csv-spec‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ FROM partial_mapping_sample_data
228228
2024-10-23T12:15:03.360Z | null | Connected to 10.1.0.3! | Disconnected from 10.1.0.3
229229
;
230230

231-
repeatedInsistFieldsAreSilentlyIgnored
231+
repeatedInsistFieldsUseTheLastEntry
232232
required_capability: unmapped_fields
233233
FROM partial_mapping_sample_data
234234
| INSIST_🐔 unmapped_message, foo, message, foo, message, unmapped_message
@@ -246,6 +246,10 @@ FROM partial_mapping_sample_data
246246
2024-10-23T12:15:03.360Z | null | Connected to 10.1.0.3! | Disconnected from 10.1.0.3
247247
;
248248

249+
#####################
250+
# Multi index tests #
251+
#####################
252+
249253
mixedFieldsMultiParametersMultiIndex
250254
required_capability: unmapped_fields
251255
required_capability: index_metadata_field
@@ -299,10 +303,6 @@ sample_data | 2023-10-23T12:27:28.948Z | null | Connected
299303
sample_data | 2023-10-23T12:15:03.360Z | null | Connected to 10.1.0.3 | null
300304
;
301305

302-
#####################
303-
# Multi index tests #
304-
#####################
305-
306306
fieldDoesNotExistMultiIndex
307307
required_capability: index_metadata_field
308308
required_capability: unmapped_fields
@@ -356,7 +356,7 @@ FROM partial_mapping_sample_data, sample_data METADATA _index
356356
;
357357

358358

359-
fieldIsMappedToDifferentTypesNoCastMultiIndex
359+
fieldIsMappedToDifferentTypesMultiIndex
360360
required_capability: index_metadata_field
361361
required_capability: unmapped_fields
362362
FROM sample_data_ts_long, sample_data METADATA _index
@@ -565,7 +565,7 @@ FROM partial_mapping_sample_data,partial_mapping_excluded_source_sample_data MET
565565
2024-10-23T13:55:01.543Z | partial_mapping_excluded_source_sample_data | null
566566
;
567567

568-
partialMappingUnionTypesStats
568+
partialMappingStatsAfterCast
569569
required_capability: index_metadata_field
570570
required_capability: source_field_mapping
571571
required_capability: unmapped_fields

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -685,38 +685,42 @@ private List<Attribute> resolveUsingColumns(List<Attribute> cols, List<Attribute
685685
}
686686

687687
private LogicalPlan resolveInsist(Insist insist, List<Attribute> childrenOutput, IndexResolution indexResolution) {
688-
return insist.withAttributes(
689-
insist.insistedAttributes().stream().map(a -> resolveInsistAttribute(a, childrenOutput, indexResolution)).toList()
690-
);
688+
List<Attribute> list = new ArrayList<>();
689+
for (Attribute a : insist.insistedAttributes()) {
690+
list.add(resolveInsistAttribute(a, childrenOutput, indexResolution));
691+
}
692+
return insist.withAttributes(list);
691693
}
692694

693695
private Attribute resolveInsistAttribute(Attribute attribute, List<Attribute> childrenOutput, IndexResolution indexResolution) {
694-
assert attribute instanceof UnresolvedAttribute : "INSIST should be unresolved at this point";
695696
Attribute resolvedCol = maybeResolveAttribute((UnresolvedAttribute) attribute, childrenOutput);
696697
// Field isn't mapped anywhere.
697698
if (resolvedCol instanceof UnresolvedAttribute) {
698699
return insistKeyword(attribute);
699700
}
700701

701-
assert resolvedCol instanceof FieldAttribute : "Resolved INSIST attribute should resolve to a field attribute";
702-
var field = ((FieldAttribute) resolvedCol).field();
703-
String name = resolvedCol.name();
704702
// Field is partially unmapped.
705-
if (indexResolution.get().isPartiallyUnmappedField(name)) {
706-
return field.getDataType() == KEYWORD
707-
? insistKeyword(attribute)
708-
: invalidInsistAttribute(attribute, (FieldAttribute) resolvedCol);
703+
if (resolvedCol instanceof FieldAttribute fa && indexResolution.get().isPartiallyUnmappedField(fa.name())) {
704+
return fa.dataType() == KEYWORD ? insistKeyword(fa) : invalidInsistAttribute(fa);
709705
}
710706

711-
// Field is mapped everywhere; we can safely ignore the INSIST command.
707+
// Either the field is mapped everywhere and we can just use the resolved column, or the INSIST clause isn't on top of a FROM
708+
// clause—for example, it might be on top of a ROW clause—so the verifier will catch it and fail.
712709
return resolvedCol;
713710
}
714711

715-
private static FieldAttribute invalidInsistAttribute(Attribute attribute, FieldAttribute fa) {
716-
String name = fa.name();
717-
var messageFormat = "Cannot use field [%s] due to ambiguities caused by INSIST. "
718-
+ "Unmapped fields are treated as KEYWORD in unmapped indices, but field is mapped to another type";
719-
return new FieldAttribute(attribute.source(), name, new InvalidMappedField(name, Strings.format(messageFormat, name)));
712+
private static Attribute invalidInsistAttribute(FieldAttribute fa) {
713+
var name = fa.name();
714+
EsField field = fa.field() instanceof InvalidMappedField imf
715+
? new InvalidMappedField(name, InvalidMappedField.makeErrorsMessageIncludingInsistKeyword(imf.getTypesToIndices()))
716+
: new InvalidMappedField(
717+
name,
718+
Strings.format(
719+
"mapped as [2] incompatible types: [keyword] enforced by INSIST command, and [%s] in index mappings",
720+
fa.dataType().typeName()
721+
)
722+
);
723+
return new FieldAttribute(fa.source(), name, field);
720724
}
721725

722726
private static FieldAttribute insistKeyword(Attribute attribute) {

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,11 @@ private static void checkBinaryComparison(LogicalPlan p, Failures failures) {
235235
}
236236

237237
private static void checkInsist(LogicalPlan p, Failures failures) {
238-
if (p instanceof Insist i && (false == (i.child() instanceof EsRelation || i.child() instanceof Insist))) {
239-
failures.add(fail(i, "[insist] can only be used after [from] or [insist] commands, but was [{}]", i.child().sourceText()));
238+
if (p instanceof Insist i) {
239+
LogicalPlan child = i.child();
240+
if ((child instanceof EsRelation || child instanceof Insist) == false) {
241+
failures.add(fail(i, "[insist] can only be used after [from] or [insist] commands, but was [{}]", child.sourceText()));
242+
}
240243
}
241244
}
242245

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,6 @@ public record EsIndex(
3333
assert partiallyUnmappedFields != null;
3434
}
3535

36-
public static EsIndex withStandardIndexMode(String name) {
37-
return new EsIndex(name, Map.of(), Map.of(name, IndexMode.STANDARD), Set.of());
38-
}
39-
40-
public static EsIndex emptyIndex(String name) {
41-
return new EsIndex(name, Map.of(), Map.of(), Set.of());
42-
}
43-
44-
/** Intended for tests. Returns an index with no partially unmapped fields. */
4536
public EsIndex(String name, Map<String, EsField> mapping, Map<String, IndexMode> indexNameWithModes) {
4637
this(name, mapping, indexNameWithModes, Set.of());
4738
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,13 @@
2222
import org.elasticsearch.xpack.esql.optimizer.rules.logical.ExtractAggregateCommonFilter;
2323
import org.elasticsearch.xpack.esql.optimizer.rules.logical.FoldNull;
2424
import org.elasticsearch.xpack.esql.optimizer.rules.logical.LiteralsOnTheRight;
25-
import org.elasticsearch.xpack.esql.optimizer.rules.logical.MergeUnmappedFields;
2625
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PartiallyFoldCase;
2726
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEmptyRelation;
2827
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEquals;
2928
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEvalFoldables;
3029
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateInlineEvals;
3130
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateNullable;
31+
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropgateUnmappedFields;
3232
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneColumns;
3333
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyPlans;
3434
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters;
@@ -196,6 +196,6 @@ protected static Batch<LogicalPlan> operators() {
196196
}
197197

198198
protected static Batch<LogicalPlan> cleanup() {
199-
return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation(), new MergeUnmappedFields());
199+
return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation(), new PropgateUnmappedFields());
200200
}
201201
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/MergeUnmappedFields.java‎ renamed to ‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropgateUnmappedFields.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
1616
import org.elasticsearch.xpack.esql.rule.Rule;
1717

18+
import java.util.ArrayList;
19+
1820
/**
1921
* Merges unmapped fields into the output of the ES relation. This marking is necessary for the block loaders to force loading from _source
2022
* if the field is unmapped.
2123
*/
22-
public class MergeUnmappedFields extends Rule<LogicalPlan, LogicalPlan> {
24+
public class PropgateUnmappedFields extends Rule<LogicalPlan, LogicalPlan> {
2325
@Override
2426
public LogicalPlan apply(LogicalPlan logicalPlan) {
2527
if (logicalPlan instanceof EsRelation) {
@@ -35,7 +37,7 @@ public LogicalPlan apply(LogicalPlan logicalPlan) {
3537
? logicalPlan
3638
: logicalPlan.transformUp(
3739
EsRelation.class,
38-
er -> er.withAttributes(NamedExpressions.mergeOutputAttributes(unmappedFields.stream().toList(), er.output()))
40+
er -> er.withAttributes(NamedExpressions.mergeOutputAttributes(new ArrayList<>(unmappedFields), er.output()))
3941
);
4042
}
4143
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -290,18 +290,15 @@ public LogicalPlan visitFromCommand(EsqlBaseParser.FromCommandContext ctx) {
290290
public PlanFactory visitInsistCommand(EsqlBaseParser.InsistCommandContext ctx) {
291291
var source = source(ctx);
292292
List<NamedExpression> fields = visitQualifiedNamePatterns(ctx.qualifiedNamePatterns(), ne -> {
293-
Source neSource = ne.source();
294-
if (ne instanceof UnresolvedStar) {
295-
throw new ParsingException(neSource, "Cannot specify [*] with INSIST", neSource.text());
296-
}
297-
if (ne instanceof UnresolvedNamePattern) {
298-
throw new ParsingException(neSource, "Cannot use wildcards ([*]) with INSIST", neSource.text());
293+
if (ne instanceof UnresolvedStar || ne instanceof UnresolvedNamePattern) {
294+
Source neSource = ne.source();
295+
throw new ParsingException(neSource, "INSIST doesn't support wildcards, found [{}]", neSource.text());
299296
}
300297
});
301298
return input -> new Insist(
302299
source,
303-
fields.stream().map(ne -> (Attribute) new UnresolvedAttribute(ne.source(), ne.name())).toList(),
304-
input
300+
input,
301+
fields.stream().map(ne -> (Attribute) new UnresolvedAttribute(ne.source(), ne.name())).toList()
305302
);
306303
}
307304

0 commit comments

Comments
 (0)