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 no 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)

He already LGTMed, but can't approve right now

astefan · 2025-02-13T12:41:47Z

@elasticsearchmachine run elasticsearch-ci/part-1

elasticsearchmachine · 2025-02-13T22:29:50Z

💔 Backport failed

Status	Branch	Result
❌	9.0	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 119886

bpintea · 2025-02-17T15:38:05Z

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

+    private static void checkInsist(LogicalPlan p, Failures failures) {
+        if (p instanceof Insist i) {
+            LogicalPlan child = i.child();
+            if ((child instanceof EsRelation || child instanceof Insist) == false) {
+                failures.add(fail(i, "[insist] can only be used after [from] or [insist] commands, but was [{}]", child.sourceText()));
+            }
+        }
+    }
+


One more piece that could be refactored: have Insist implement PostAnalysisVerificationAware and move the check there.
Nit: I think we capitalise the commands elsewhere (including tests for this check), we should do the same here.

This PR adds initial support for unmapped fields, using the INSIST clause. For starters, this unmapped fields without a cast. 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. As this is currently under development, the actual syntax is INSIST_🐔. First stage of elastic#120072. Specifically, the following features are implemented in this PR: * Support for INSIST keyword 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 no support for union type resolution on top of INSIST. Future PRs will handle these conflicts. There is support for multiple parameters, or INSIST on top of INSIST which is equivalent. * Enforcing that INSIST must always be on top of a FROM or another INSIST. While this may change in the future, e.g., handling cases like `FROM foo | EVAL x = 1 | INSIST bar` will not be done in this PR, as it makes handling INSIST too complicated.

#121261, #122607) (#122805) * ESQL: Support for _index metadata field in CsvTests (#121261) * ESQL: Support for _index metadata field in CsvTests * Extract INDEX constant to MetadataAttribute * Add comment on capability * ESQL: Initial support for unmapped fields (#119886) This PR adds initial support for unmapped fields, using the INSIST clause. For starters, this unmapped fields without a cast. 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. As this is currently under development, the actual syntax is INSIST_🐔. First stage of #120072. Specifically, the following features are implemented in this PR: * Support for INSIST keyword 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 no support for union type resolution on top of INSIST. Future PRs will handle these conflicts. There is support for multiple parameters, or INSIST on top of INSIST which is equivalent. * Enforcing that INSIST must always be on top of a FROM or another INSIST. While this may change in the future, e.g., handling cases like `FROM foo | EVAL x = 1 | INSIST bar` will not be done in this PR, as it makes handling INSIST too complicated.

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

GalLalouche enabled auto-merge (squash) February 12, 2025 13:05

GalLalouche added 6 commits February 12, 2025 15:06

Merge branch 'main' into feature/unmapped_fields_squashed

c9fdc92

Merge branch 'main' into feature/unmapped_fields_squashed

b46cb9d

Merge branch 'main' into feature/unmapped_fields_squashed

b5da964

Merge branch 'main' into feature/unmapped_fields_squashed

7a48277

Merge branch 'main' into feature/unmapped_fields_squashed

dcc518c

Merge branch 'main' into feature/unmapped_fields_squashed

770ea6f

GalLalouche added v9.0.0 auto-backport Automatically create backport pull requests when merged v9.0.1 and removed auto-backport Automatically create backport pull requests when merged v8.19.0 v9.0.0 labels Feb 13, 2025

GalLalouche removed the test-release Trigger CI checks against release build label Feb 13, 2025

GalLalouche added 2 commits February 13, 2025 14:41

Merge branch 'main' into feature/unmapped_fields_squashed

97399fc

Merge branch 'main' into feature/unmapped_fields_squashed

e32a446

GalLalouche merged commit d3ed999 into elastic:main Feb 13, 2025
17 checks passed

elasticsearchmachine added the backport pending label Feb 13, 2025

luigidellaquila mentioned this pull request Feb 14, 2025

ES|QL: disable mixed cluster CSV tests that require source field mapping #122607

Merged

bpintea reviewed Feb 17, 2025

View reviewed changes

This was referenced Feb 17, 2025

[9.0] Initial support for unmapped fields (Plus a few others) (#119886, #121261, #122607) #122804

Closed

[9.0] Initial support for unmapped fields (Plus a few others) (#119886, #121261, #122607) #122805

Merged

ioanatia mentioned this pull request Feb 27, 2025

[CI] EsqlSpecIT class failing #121411

Closed

nik9000 removed the backport pending label Apr 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ESQL: Initial support for unmapped fields #119886

ESQL: Initial support for unmapped fields #119886

Uh oh!

GalLalouche commented Jan 9, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented Jan 12, 2025

Uh oh!

elasticsearchmachine commented Jan 12, 2025

Uh oh!

alex-spies left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

astefan left a comment

Uh oh!

astefan commented Feb 13, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2025

Uh oh!

bpintea Feb 17, 2025

Uh oh!

Uh oh!

ESQL: Initial support for unmapped fields #119886

ESQL: Initial support for unmapped fields #119886

Uh oh!

Conversation

GalLalouche commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 12, 2025

Uh oh!

elasticsearchmachine commented Jan 12, 2025

Uh oh!

alex-spies left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

astefan commented Feb 13, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2025

💔 Backport failed

Uh oh!

bpintea Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GalLalouche commented Jan 9, 2025 •

edited

Loading

alex-spies left a comment •

edited

Loading