ESQL: Initial support for unmapped fields #119886

GalLalouche · 2025-01-09T16:46:16Z

This PR adds initial support for unmapped fields, using the INSIST clause. For starters, this only support a single unmapped field, without casting. Casting may appear after the INSIST clause, similar to union types.
Note that the INSIST keyword is potentially a placeholder, as the method of defining an unmapped field might change in the future, e.g., use a special magic function.

First stage of #120072.

Specifically, the following features are implemented in this PR:

Support for INSIST keyword and a single parameter without a cast. In particular, if the type being INSISTed upon is mapped to anything other than KEYWORD, it will result in an InvalidMappedField. There is not support for union type resolution on top of INSIST. Future PRs will handle these conflicts.
Enforcing that INSIST must always be on top of a FROM. While this may change in the future, e.g., in cases like FROM foo | EVAL x = 1 | INSIST bar, it will not be done in this PR, as it makes handling INSIST too complicated.

elasticsearchmachine · 2025-01-12T14:03:53Z

Hi @GalLalouche, I've created a changelog YAML for you.

elasticsearchmachine · 2025-01-12T14:04:40Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

alex-spies

Heya! I only took a quick glance at this, but I noticed the following:

~~The new syntax and command are immediately released. Can we please hide this behind a SNAPSHOT-only? Otherwise it's gonna be made available in Serverless on the next rollout.~~ Update: sorry, it's already hidden behind SNAPSHOT in the parser, so that's good.
I can see some unrelated parser files being changed (SQL, EQL, KQL and Painless). I guess these were accidentally changed due to regenerating the parsers? I'd like to exclude these from the PR, please.
If at all possible, it'd be great to include a csv test to showcase the functionality. Or a yaml test if csv tests are too inflexible. In case that'd blow the PR's scope because not all is in place to run the feature end-to-end, let's at least add a short javadoc to the Insist class to describe its purpose. Update: Sorry, you correctly added a csv-spec test which I missed.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

...k/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/CollectionUtils.java

astefan

Some quick comments, without going deep with the code:

disabling the _source should be covered so that whatever we do know should still happen after this PR
excluding certain fields from _source behavior should also be kept as is. If we already have tests for this somewhere in yaml, csv or other IT tests, that's great and apologies for mentioning it.
there is also the scenario where index: false and doc_values: false can be configured for a certain field. In this case, I guess this PR should address loading the value from _source as well (if _source is available, of course, considering my previous two use cases)

astefan

Why is there an error presented when using insist with an existent field? For example, FROM employees | insist languages | stats count(*) by languages

languages is a numeric field and this field exists only in this index, so insist here shouldn't do anything imo. This is not a casting operation to keyword, as the error message seems to imply. I would have expected a warning message at most saying something like "there are no unmapped fields called [languages]".

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

astefan · 2025-01-17T10:39:30Z

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

+fieldIsMappedToNonKeywordWithCastSingleIndex
+required_capability: unmapped_fields
+FROM partial_mapping_sample_data
+| INSIST client_ip


The more I look at how this command is used (and I understand the PR is doing the bare minimum) the more I see the need of an explicit mapping ie insist client_ip::keyword (keyword is the default right now).

I would really prefer to have this in a future PR, and desugar INSIST foo :: x into INSIST foo | EVAL foo = foo :: x.

From internal discussions regarding this functionality I understood that we want to start simple with FROM logs | INSIST field::LONG (the explicit casting is there) and that this command has a different equivalent than the one you specified there: FROM logs | EVAL field = CASE(IS_MAPPED("field"), field, EXTRACT_FROM_SOURCE("field"))::LONG. Meaning, if the field already exists and there is no conflict, we don't force the extraction.

See my comment here regarding check if field exists or not: #119886 (comment)

GalLalouche · 2025-01-17T10:46:48Z

This is not a casting operation to keyword, as the error message seems to imply. I would have expected a warning message at most saying something like "there are no unmapped fields called [languages]".

Unfortunately, we don't currently keep any information on which index has the required mappings, and which don't (we only keep it in case of mapping conflicts). I would prefer to tackle the exact behavior of this edge case in another PR, as this would require a pretty large change of our index resolution.

astefan · 2025-01-17T10:50:42Z

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

+fieldDoesNotExistSingleIndex
+required_capability: unmapped_fields
+FROM partial_mapping_sample_data
+| INSIST foo


Having a query like FROM partial_mapping_sample_data | eval x = 1 | INSIST client_ip and getting an error message is kind of unexpected:

the error message doesn't simply say what's wrong, ie insist must come after from somehow: "line 1:49: INSIST command can only be applied on top of a source". Maybe you can improve this with .... on top of a source command (from, row etc).

But does this work with row? I tested row a = 1 | INSIST client_ip and I get class org.elasticsearch.xpack.esql.plan.logical.Row cannot be cast to class org.elasticsearch.xpack.esql.plan.logical.EsRelation

that eval x = 1 has nothing to do with insist and also doesn't change the query in any way

Sure, I'll change the message.

I'll fix the bug with ROW.

You're right that the EVAL x = 1 has nothing to do with INSIST, but it makes the code much easier if I don't have to consider all the various places INSIST can appear in :) As this syntax is a work in progress, I'd rather keep this requirement as is for now, and revisit it later if needed.

On further thought, should we really allow for ROW? It's almost certainly a mistake, as the INSIST command is a noop in that case. I think it would make more sense to forbid erroneous code than to silently ignore it, or even warn about it. WDYT?

astefan · 2025-01-17T11:41:29Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

@@ -2358,4 +2358,8 @@ public void testFailingMetadataWithSquareBrackets() {
            "line 1:11: mismatched input '[' expecting {<EOF>, '|', ',', 'metadata'}"


I think it would be good for this class to have more tests for what insist supports and what not. In the future when the restrictions on this command will ease, it would be easier to check them. For example insist foo, foo or insist foo*.

I've added a test for foo* and a (temporary) test for foo, bar. As for foo, foo, I'm thinking we should allow it, since we also allow KEEP foo, foo (Although I have no idea why...). WDYT?

astefan · 2025-01-17T13:44:40Z

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

+import java.io.IOException;
+
+// To make things simpler, we're only dealing with a single INSIST parameter for now.
+public record InsistParameters(String identifier) implements NamedWriteable {


I don't see the need for a separate class just to hold a list of field names (assuming that this will be in the future a list).

astefan · 2025-01-17T13:47:53Z

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

@@ -204,4 +204,8 @@ public static void writeIndexMode(StreamOutput out, IndexMode indexMode) throws
            throw new IllegalStateException("not ready to support index mode [" + indexMode + "]");
        }
    }
+
+    public EsRelation withAttributes(List<Attribute> concat) {


concat? :-)

This is what happens when you ask Intellij to create a method 😅.

elasticsearchmachine · 2025-01-19T12:35:20Z

Hi @GalLalouche, I've updated the changelog YAML for you.

…uche/elasticsearch into feature/unmapped_fields_squashed

costin

I've left a number of comments - some low level some higher.
The main takeaways are:

rename the syntax to FIELD_🐔
use attribute (Unresolved or if needed introduce another one) and the Analyzer to perform the resolution instead of using strings in the command

Since this is a big PR, let's aim to get it to a good enough state and descope as much as possible into potential follow-ups.

P.S. Overall, try to keep the style of the existing code base; help with readability. Most of it its arbitrary and consistency is valuable - use alphabetical order where need, chronological otherwise.

server/src/main/java/org/elasticsearch/index/mapper/BlockSourceReader.java

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

costin · 2025-01-22T04:29:42Z

...ql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/PotentiallyUnmappedEsField.java

+    public sealed interface State {}
+
+    /**
+     * A field which is either unmapped in all indices, or mapped to {@link DataType#KEYWORD} in all indices where it is mapped.
+     * In either case, there is no type conflict.
+     */
+    public enum KeywordResolved implements State {
+        INSTANCE;
+    }
+
+    /**
+     * A field which is mapped to a single type, which is not {@link DataType#KEYWORD}. This can be resolved using a cast, similar to
+     * union types. This is treated differently from {@link Invalid}, since resolving this conflict doesn't use
+     * {@link MultiTypeEsField}.
+     */
+    public record SimpleConflict(DataType otherType) implements State {
+        public SimpleConflict {
+            if (otherType == KEYWORD) {
+                throw new IllegalArgumentException("Use NoConflicts.INSTANCE for KEYWORD");
+            }
+        }
+    }
+
+    /**
+     * A field which is mapped to a single type, but that type is not {@link DataType#KEYWORD}.
+     * This is resolved using the specified conversions.
+     */
+    public record SimpleResolution(Expression unmappedConversion, Expression mappedConversion) implements State {}
+
+    /**
+     * A field which is mapped to more than one type in multiple indices, *in addition* to the unmapped case which is always treated as
+     * {@link DataType#KEYWORD}. This can be resolved using a cast, similar to union types.
+     */
+    public record Invalid(InvalidMappedField invalidMappedField) implements State {}
+
+    /**
+     * A field which is mapped to different types in different indices, but resolved using union types. In mapped indices, we treat this
+     * as a union type, and use the specified conversion for unmapped indices.
+     */
+    public record MultiType(Expression conversionFromKeyword, MultiTypeEsField multiTypeEsField) implements State {}
+
+    private final State state;
+
+    public State getState() {
+        return state;
+    }


I've been struggling with this section.
Looks like PotentiallyUnmappedEsField tries to do multiple things at once:

be a container that holds the underlying data type

indicate if there's a conflict

specify a conversion expression

be an invalid mapped field

Recommend breaking the behavior in bits and using the existing field classes instead based on the Analyzer resolution:
a. have the UnmappedEsField handle only extraction from source, potentially by supporting a conversion on it as well
b. in case of invalid mapping, use InvalidMappedField.
c. if the solution is multiType, use the MultiTpeEsField instead.

As a side note, instead of using the expression inside the field type, use the actual type you want (I believe EvaluatorMapper) or add an lambda interface to it that wraps the target action.

As the PR has been descoped, these are no longer necessary in this PR.

costin · 2025-01-22T04:35:04Z

...k/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/CollectionUtils.java

@@ -83,4 +85,13 @@ public static <T> List<T> nullSafeList(T... entries) {
        }
        return list;
    }
+
+    public static <T> OptionalInt findIndex(List<T> list, Predicate<T> predicate) {


The current style of ESQL limits the use of functional constructs and Optional and instead uses more of a C style approach and use an out of band number for primitives (-1) or null for objects.

for (int i = 0, s = list.size(); i < s; i++) { if (predicate.test(list,.get(i)) { return i; } } return -1;

While I get that this is the current style, I think the use of Optional is a lot safer and clearer. If this isn't a very strict requirement, I would much rather keep this as is. No reason to throw good code after bad :)

x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/util/TestUtils.java

costin · 2025-01-22T04:55:17Z

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

+            if (input instanceof EsRelation || input instanceof UnresolvedRelation) {
+                return new Insist(source, visitIdentifier(ctx.identifier()), input);
+            }
+            throw new ParsingException(source, "INSIST command can only be applied on top of a FROM command.");
+        };


What is there's another insist command in front?

You mean, INSIST on top of another INSIST? Then this would fail. While we could support this kind of superfluous syntax, I don't think it should be done in this PR.

costin · 2025-01-22T04:59:42Z

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

+public class Insist extends UnaryPlan {
+    public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "INSIST", Insist::new);
+
+    private final String insistIdentifier;


Identifiers are rarely used since are fragile (renames, name spaces, etc). Use Attributes instead.

costin · 2025-01-22T05:02:56Z

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

+    private List<Attribute> computeOutput() {
+        var result = new ArrayList<>(child().output());
+        OptionalInt index = CollectionUtils.findIndex(child().output(), c -> c.name().equals(insistIdentifier));
+        index.ifPresentOrElse(i -> {
+            var field = ((FieldAttribute) child().output().get(i)).field();
+            result.set(i, new FieldAttribute(source(), insistIdentifier, PotentiallyUnmappedEsField.fromField(field)));
+        }, () -> result.add(new FieldAttribute(source(), insistIdentifier, PotentiallyUnmappedEsField.fromStandalone(insistIdentifier))));
+        return result;


Instead of doing resolution here, use an UnresolvedAttribute (or extension of it) which the Analyzer resolved to a proper field, similar to UnresolvedAttribute.
The command thus starts with a bunch of attributes which the analyzer tries to resolves and either succeeds (MultiType, etc..) or fails (Unresolved with a message).

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

GalLalouche · 2025-01-26T14:06:55Z

I've descoped a lot of the features in the PR, specifically, the resolution of union types on top of an INSIST. If there is any type conflict, i.e., if a field is mapped to anything other than KEYWORD in any index, it is no longer resolvable from within the language in this PR.

In addition, the resolved index now maintains a set of all partially mapped fields. This does not change metadata queried via field capabilities, and does not live past the analyzer, so it doesn't affect serialization.

elasticsearchmachine added the v9.0.0 label Jan 9, 2025

GalLalouche force-pushed the feature/unmapped_fields_squashed branch 7 times, most recently from 5f04c1f to fd1abd0 Compare January 12, 2025 12:18

GalLalouche added >feature Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.0 labels Jan 12, 2025

GalLalouche marked this pull request as ready for review January 12, 2025 14:04

GalLalouche requested a review from a team as a code owner January 12, 2025 14:04

GalLalouche requested review from craigtaverner and removed request for a team January 12, 2025 14:04

GalLalouche requested a review from nik9000 January 12, 2025 16:33

alex-spies requested changes Jan 13, 2025

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved

thecoop reviewed Jan 13, 2025

View reviewed changes

...k/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/CollectionUtils.java Outdated Show resolved Hide resolved

alex-spies self-requested a review January 13, 2025 11:07

GalLalouche and others added 5 commits January 13, 2025 13:49

TEMP

fa3dd20

WTF checkstyle?!

c2b0d9c

Trying to fix failures

7b33f15

Add capability, Fix parser test

a105f0a

Update docs/changelog/119886.yaml

0ca5e79

GalLalouche force-pushed the feature/unmapped_fields_squashed branch from 4289393 to 0ca5e79 Compare January 13, 2025 11:54

astefan reviewed Jan 13, 2025

View reviewed changes

Merge branch 'main' into feature/unmapped_fields_squashed

c22b9d4

astefan reviewed Jan 17, 2025

View reviewed changes

x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped_fields.csv-spec Outdated Show resolved Hide resolved

astefan reviewed Jan 17, 2025

View reviewed changes

Update docs/changelog/119886.yaml

f5c77fa

GalLalouche mentioned this pull request Jan 19, 2025

Support for unmapped fields #120072

Open

GalLalouche added 3 commits January 19, 2025 16:57

More code review fixes

d6afb24

Merge branch 'main' into feature/unmapped_fields_squashed

25f0c53

Merge branch 'feature/unmapped_fields_squashed' of github.com:GalLalo…

165ad16

…uche/elasticsearch into feature/unmapped_fields_squashed

GalLalouche requested review from alex-spies, astefan and craigtaverner January 19, 2025 15:07

costin requested changes Jan 22, 2025

View reviewed changes

GalLalouche added 4 commits January 22, 2025 15:32

Some of Costin's notes

9d85915

Pre huge refactor

d0959ea

Post huge refactor

d68a0ca

Merge branch 'main' into feature/unmapped_fields_squashed

a12aa82

GalLalouche force-pushed the feature/unmapped_fields_squashed branch from 47f568d to 9bfb1e7 Compare January 26, 2025 13:37

GalLalouche added 2 commits January 26, 2025 16:07

Temp fixes

f02fd43

Slighly more delicate refactor

a8d9372

GalLalouche force-pushed the feature/unmapped_fields_squashed branch from 9bfb1e7 to a8d9372 Compare January 26, 2025 14:07

GalLalouche requested a review from costin January 26, 2025 14:08

GalLalouche added 2 commits January 26, 2025 16:10

Fix borken test

fcd4663

Handle failing tests

f9b5171

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Initial support for unmapped fields #119886

ESQL: Initial support for unmapped fields #119886

GalLalouche commented Jan 9, 2025 •

edited

Loading

elasticsearchmachine commented Jan 12, 2025

elasticsearchmachine commented Jan 12, 2025

alex-spies left a comment •

edited

Loading

astefan left a comment

astefan left a comment

astefan Jan 17, 2025

GalLalouche Jan 17, 2025

astefan Jan 17, 2025

GalLalouche Jan 17, 2025

GalLalouche commented Jan 17, 2025 •

edited

Loading

astefan Jan 17, 2025

GalLalouche Jan 17, 2025

GalLalouche Jan 19, 2025

astefan Jan 17, 2025

GalLalouche Jan 19, 2025

astefan Jan 17, 2025

GalLalouche Jan 19, 2025

astefan Jan 17, 2025

GalLalouche Jan 19, 2025

elasticsearchmachine commented Jan 19, 2025

costin left a comment

costin Jan 22, 2025

GalLalouche Jan 26, 2025

costin Jan 22, 2025

GalLalouche Jan 22, 2025

costin Jan 22, 2025

GalLalouche Jan 22, 2025

costin Jan 22, 2025

GalLalouche Jan 26, 2025

costin Jan 22, 2025

GalLalouche Jan 26, 2025

GalLalouche commented Jan 26, 2025

		@@ -2358,4 +2358,8 @@ public void testFailingMetadataWithSquareBrackets() {
		"line 1:11: mismatched input '[' expecting {<EOF>, '\|', ',', 'metadata'}"

ESQL: Initial support for unmapped fields #119886

Are you sure you want to change the base?

ESQL: Initial support for unmapped fields #119886

Conversation

GalLalouche commented Jan 9, 2025 • edited Loading

elasticsearchmachine commented Jan 12, 2025

elasticsearchmachine commented Jan 12, 2025

alex-spies left a comment • edited Loading

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalLalouche commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 19, 2025

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalLalouche commented Jan 26, 2025

GalLalouche commented Jan 9, 2025 •

edited

Loading

alex-spies left a comment •

edited

Loading

GalLalouche commented Jan 17, 2025 •

edited

Loading