Disallow unmapped_fields=load with partial non-KEYWORD by shmuelhanoch · Pull Request #144109 · elastic/elasticsearch

shmuelhanoch · 2026-03-12T15:33:08Z

Fail the query when SET unmapped_fields="load" is used and any index has a field that is partially mapped (present in some indices, unmapped in others) and whose type where mapped is not KEYWORD. Add analyzer rule DisallowLoadWithPartiallyMappedNonKeyword and unit tests.

Closes #143218

Fail the query when SET unmapped_fields="load" is used and any index has a field that is partially mapped (present in some indices, unmapped in others) and whose type where mapped is not KEYWORD. Add analyzer rule DisallowLoadWithPartiallyMappedNonKeyword and unit tests. Closes elastic#143218

elasticsearchmachine · 2026-03-16T14:26:55Z

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

alex-spies

Thanks @shmuelhanoch ! I think the solution is generally correct, but I have some remarks around where to put it and how to test it.

In particular, I think this requires some yaml tests to confirm the behavior is correct in end-to-end tests as well, because our unit tests are a little "academic" and require us to mimic how we create mappings very well.

...a/org/elasticsearch/xpack/esql/analysis/rules/DisallowLoadWithPartiallyMappedNonKeyword.java

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

alex-spies · 2026-03-18T17:56:53Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

I think we should add a bunch more test cases. In particular, tests where a partially mapped field is referenced not (just) in KEEP, but also, say, WHERE.

We have WHERE covered now. Great! Let's do more different commands and expressions.

Like, if someone accidentally slipped in a regression that bypasses this verification for, say, CHANGE_POINT, we would want a test to fail. That's true for all commands.

...a/org/elasticsearch/xpack/esql/analysis/rules/DisallowLoadWithPartiallyMappedNonKeyword.java

alex-spies · 2026-03-18T18:42:58Z

...a/org/elasticsearch/xpack/esql/analysis/rules/DisallowLoadWithPartiallyMappedNonKeyword.java

+     * Resolve a dotted field path (e.g. "a.b.c") in the given root mapping.
+     * Returns the leaf EsField or null if any segment is missing.
+     */
+    private static EsField resolveFieldByPath(Map<String, EsField> root, String path) {


Given that we add a bunch of code to deal with subfields, we should also add a bunch of tests with subfields.

alex-spies · 2026-03-18T18:50:49Z

...a/org/elasticsearch/xpack/esql/analysis/rules/DisallowLoadWithPartiallyMappedNonKeyword.java

+                    resolutionsInPlan.add(r);
+                }
+            } else {
+                IndexResolution r = context.indexResolution().get(new IndexPattern(relation.source(), relation.indexPattern()));


When do we encounter a case with more than 1 index resolution?

How do we test this?

…fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

alex-spies · 2026-03-25T12:49:39Z

From discussing with @GalLalouche it occurred to me that we should particularly test date/nanos union types:

SET unmapped_fields="load";
FROM sample_data, sample_data_ts_nanos, no_mapping_sample_data METADATA _index
| WHERE @timestamp == "2021-01-01"::date_nanos

Here, @timestamp is date in sample_data, date_nanos in sample_data_ts_nanos, and outright unmapped in no_mapping_sample_data.

The peculiarity is that, without load, our analyzer will implicitly cast @timestamp to date_nanos. That could mess with the behavior we are implementing.

The expected behavior should still be that we fail the query because @timestamp is partially unmapped and used without a cast. (The case with an explicit cast should work but is out of scope here, and will be either part of #143693 or a follow-up.)

Fail the query when SET unmapped_fields="load" is used and any index has a field that is partially mapped (present in some indices, unmapped in others) and whose type where mapped is not KEYWORD. Add analyzer rule DisallowLoadWithPartiallyMappedNonKeyword and unit tests. Closes elastic#143218

- Move partially-unmapped non-KEYWORD load validation from the analyzer Resolution batch into Verifier (after existing load-mode checks). - Fail only when those fields appear in FieldAttributes outside EsRelation (actually used by the query). - Resolve indices from non-LOOKUP EsRelations only; pass AnalyzerContext for indexResolution() lookups. - Remove DisallowLoadWithPartiallyMappedNonKeyword; add LoadPartialMappingChecks and a Verifier.verify overload that accepts AnalyzerContext. - Extend AnalyzerUnmappedTests: FROM-only, WHERE, SORT, comma-separated indices, dotted paths, nullify. - Add esql YAML REST tests for load vs partial non-KEYWORD (used vs unused) behind optional_fields_v2.

…fields-load

astefan · 2026-03-26T07:00:54Z

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


    public LogicalPlan verify(LogicalPlan plan, BitSet partialMetrics) {
-        Collection<Failure> failures = verifier.verify(plan, partialMetrics, context().unmappedResolution());
+        Collection<Failure> failures = verifier.verify(plan, partialMetrics, context().unmappedResolution(), context());


Here you don't need to pass context().unmappedResolution() and context().
context() is sufficient, you can extract the unmappedResolution() in the method itself.

nit: Do we need to pass on the whole context? It's not wrong, but it makes it may make it look like we need all kinds of things in the verifier, but we only really need the unmapped resolution and indexResolution, no?

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

astefan · 2026-03-26T07:25:54Z

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

            checkLoadModeDisallowedCommands(plan, failures);
            checkLoadModeDisallowedFunctions(plan, failures);
            checkFlattenedSubFieldLoad(plan, failures);
+            if (failures.hasFailures() == false) {


Can you explain why is this check?
In ESQL, in general, we try to present to the user the full list of verification failures in one batch. For example, AnalyzerTests.testLookupJoinUnknownField and AnalyzerUnmappedTests.testTbucketWithUnmappedTimestampWithFork.

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/LoadPartialMappingChecks.java

astefan · 2026-03-26T07:44:45Z

...src/yamlRestTest/resources/rest-api-spec/test/esql/192_unmapped_load_partial_non_keyword.yml

+
+  - match: { status: 400 }
+  - match: { error.type: verification_exception }
+  - match: { error.reason: "/partial_x/" }


Let's assert the full error message, please.

I am not sure what the intention with this test is. It's throwing

Cannot use field [partial_x] due to ambiguities being mapped as [2] incompatible types: [keyword] enforced by INSIST command, and [long] in index mappings

I was expecting the error message from LoadPartialMappingChecks.checkPartialNonKeywordWithLoad... I guess.

There is code in ResolveRefs which turns partial_x into a type with a mapping conflict (just like in union types) and attaches this error message. It's code that I remove in #144228 (but #143693 may need it after all, will find out when resolving merge conflicts).

However, I'd also expect to see the new error message. I think we may not see it because the new validation is in a branch with this condition:

if (failures.hasFailures() == false) {

astefan · 2026-03-26T08:11:04Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

+    public void testDisallowLoadWithPartiallyMappedNonKeyword() {
+        assumeTrue("Requires OPTIONAL_FIELDS_V2", EsqlCapabilities.Cap.OPTIONAL_FIELDS_V2.isEnabled());
+
+        var esIndex = new EsIndex(


I think tests that live in VerifierTests are better suited for these tests. The advantage with VerifierTests is that it's not bypassing some steps... here you are manually building EsIndex instances and it's kind of fragile imho.

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

astefan

Actually, I may be wrong, but the code in LoadPartialMappingChecks can be simpler than what is currently proposed. Notice, also, that I shortened the error message. Imho, it's too long and the second part of it can be omitted. Up to you and @alex-spies if that's the case or not.

static void checkPartialNonKeywordWithLoad(LogicalPlan plan, @Nullable AnalyzerContext context, Failures failures) {
        if (context == null || context.unmappedResolution() != UnmappedResolution.LOAD) {
            return;
        }

        Set<String> nonKeywordFieldNames = new HashSet<>();
        List<IndexResolution> resolutions = new ArrayList<>();

        plan.forEachDown(LogicalPlan.class, p -> {
            if (p instanceof EsRelation rel) {
                if (rel.indexMode() != IndexMode.LOOKUP) {
                    IndexResolution r = context.indexResolution().get(new IndexPattern(rel.source(), rel.indexPattern()));
                    if (r != null) {
                        resolutions.add(r);
                    }
                }
            } else {
                p.forEachExpression(FieldAttribute.class, fa -> {
                    if (fa.dataType() != KEYWORD) {
                        nonKeywordFieldNames.add(fa.name());
                    }
                });
            }
        });

        for (String fieldName : new TreeSet<>(nonKeywordFieldNames)) {
            for (IndexResolution resolution : resolutions) {
                if (resolution.isValid() && resolution.get().isPartiallyUnmappedField(fieldName)) {
                    failures.add(
                        fail(
                            plan,
                            "Loading partially mapped non-KEYWORD fields is not yet supported. "
                                + "unmapped_fields=\"load\" is not supported with partially mapped field [{}].",
                            fieldName
                        )
                    );
                    break;
                }
            }
        }
    }

…fields-load

- Refactor repeated partial-index test setup in AnalyzerUnmappedTests using shared helpers for index and field construction. - Assert the full load partial-non-keyword verification message in relevant AnalyzerUnmappedTests scenarios. - Update YAML REST coverage to exercise the intended load verification path and match the full error.reason payload.

alex-spies

Heya, had another look. The simplification suggestion from @astefan is valid.

We still should add more tests, they help a lot esp. when we have many in-flight PRs that may affect each other. I added some suggestions.

Finally, for the questions of whether KEEP/DROP should be permitted: I commented that I formed an opinion and that is that KEEP partial_long/KEEP partial_l* (and resp. for DROP) should be okay, because it's also okay for regular mapped fields with mapping conflicts. I'm actually adding tests for this in #144228, so this PR can try to add tests for this but will likely run into problems solved in #144228, as the current state of the code very eagerly turns partially unmapped non-KEYWORD fields into field attributes with mapping conflicts when they're mentioned anywhere.

alex-spies · 2026-03-26T13:24:21Z

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


    public LogicalPlan verify(LogicalPlan plan, BitSet partialMetrics) {
-        Collection<Failure> failures = verifier.verify(plan, partialMetrics, context().unmappedResolution());
+        Collection<Failure> failures = verifier.verify(plan, partialMetrics, context().unmappedResolution(), context());


nit: Do we need to pass on the whole context? It's not wrong, but it makes it may make it look like we need all kinds of things in the verifier, but we only really need the unmapped resolution and indexResolution, no?

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

alex-spies · 2026-03-26T14:40:39Z

...src/yamlRestTest/resources/rest-api-spec/test/esql/192_unmapped_load_partial_non_keyword.yml

+        - method: POST
+          path: /_query
+          parameters: [ ]
+          capabilities: [ optional_fields_v2 ]


Since we're testing a behavior that wasn't in place on older nodes, I'm almost certain this PR will need a new capability that's used here. It's also fine to bump optional_fields_v2 to optional_fields_v3 (or actually - likely v4 or v5, depending on which of #144112, #143693 and #144228 get merged, first) as long as the feature is SNAPSHOT-only.

alex-spies · 2026-03-26T14:45:24Z

...src/yamlRestTest/resources/rest-api-spec/test/esql/192_unmapped_load_partial_non_keyword.yml

+
+  - match: { status: 400 }
+  - match: { error.type: verification_exception }
+  - match: { error.reason: "/partial_x/" }


There is code in ResolveRefs which turns partial_x into a type with a mapping conflict (just like in union types) and attaches this error message. It's code that I remove in #144228 (but #143693 may need it after all, will find out when resolving merge conflicts).

However, I'd also expect to see the new error message. I think we may not see it because the new validation is in a branch with this condition:

if (failures.hasFailures() == false) {

alex-spies · 2026-03-26T14:57:24Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

+        var esIndex = new EsIndex(
+            "partial_idx",
+            Map.of("partial_long", new EsField("partial_long", DataType.LONG, emptyMap(), true, EsField.TimeSeriesFieldType.NONE)),
+            Map.of("partial_idx", IndexMode.STANDARD),


nit: only 1 index is not realistic if you want a partial mapping in production situations (except for index aliases, but you still have to resolve that alias to 2 separate shards).

I'd instead make this Map.of("partial_idx1", IndexMode.STANDARD, "partial_idx2", IndexMode.STANDARD) and use FROM partial_idx* in the query below.

alex-spies · 2026-03-26T15:12:54Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

+        );
+        assertUnmappedLoadError(
+            analyzer().addIndex(esIndex),
+            "FROM partial_idx | KEEP partial_long",


Sorry, we discussed this offline and I formed an opinion on the KEEP/DROP question. IMO, KEEP/DROP should be allowed, but no usage in other commands, or in expressions except for inside convert functions.

Tests with just KEEP/DROP partial_long don't have to be covered here; let's instead use e.g. WHERE partial_long > 2 or so. I'm adding some positive tests for KEEP partial_... and DROP partial_... in #144228. We can merge this first and leave this edge case up to #144228 to include in another iteration.

alex-spies · 2026-03-26T15:28:29Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

+            Set.of("partial_long")
+        );
+        var plan = analyzer().addIndex(esIndex).statement(setUnmappedLoad("FROM partial_idx | KEEP common"));
+        assertThat(plan, not(nullValue()));


nit: It's better to assert the plan structure, or use a golden test.

alex-spies · 2026-03-26T15:36:28Z

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

We have WHERE covered now. Great! Let's do more different commands and expressions.

Like, if someone accidentally slipped in a regression that bypasses this verification for, say, CHANGE_POINT, we would want a test to fail. That's true for all commands.

alex-spies · 2026-03-26T15:47:09Z

...src/yamlRestTest/resources/rest-api-spec/test/esql/192_unmapped_load_partial_non_keyword.yml

      esql.query:
        body:
-          query: 'SET unmapped_fields="load"; FROM partial_load_yaml_* | WHERE partial_x > 0 | KEEP tag'
+          query: 'SET unmapped_fields="load"; FROM partial_load_yaml_* | KEEP partial_x'


Sorry, I think the original query was fine! The KEEP partial_x one will be considered valid with #144228.

…fields-load

…-load' into 143218-esql-unmapped-fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

- Move the detection and rejection of partially unmapped non-keywords to Verifier

alex-spies

Heya, the productive changes look good to me with minor comments. Mostly, I'm unsure if subfields are handled correctly and would suggest this gets thoroughly tested.

Please, consider this unblocked from my end; let's address the remaining comments (esp. around additional testing) and . If he's available, please seek final review from @astefan as I'll be only back on Monday.

Thank you @shmuelhanoch and @mouhc1ne , as well as @astefan for the thorough first review round!

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

- Minor change in how we collect PUNKs: it now avoids string comparisons and relies on es relation output and attribute sets.

…fields-load

…fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

…fields-load

- AnalyzerUnmappedTests: Bring back several unit tests, originally added by Shmuel, but accidentally removed by me during merge conflicts that occurred as I was updating the branch. These tests are failing now, pushing nonetheless so we're all on the same baseline. - Verifier: EsIndex changed the way it tells PUNKs, so we adjust.

…fields-load

…-load' into 143218-esql-unmapped-fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

…fields-load

- Minor adjustments to unit test: assert number of problems, and line/col number. - Revert back the Verifier's PUNK detection to the earlier string based approach with field names, instead of the field attributes approach which changed with Gal's PR.

…fields-load

elasticsearchmachine added the v9.4.0 label Mar 12, 2026

shmuelhanoch added >non-issue :Analytics/ES|QL AKA ESQL and removed v9.4.0 labels Mar 12, 2026

shmuelhanoch self-assigned this Mar 12, 2026

shmuelhanoch added the v7.9.4 label Mar 12, 2026

shmuelhanoch force-pushed the 143218-esql-unmapped-fields-load branch from 8b4069d to fee6a30 Compare March 16, 2026 03:31

shmuelhanoch force-pushed the 143218-esql-unmapped-fields-load branch from 5f76a74 to 90769e3 Compare March 16, 2026 14:22

shmuelhanoch marked this pull request as ready for review March 16, 2026 14:26

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 16, 2026

[CI] Auto commit changes from spotless

d26b416

alex-spies self-requested a review March 16, 2026 14:35

alex-spies mentioned this pull request Mar 18, 2026

DO NOT MERGE: ESQL: Unmapped load preview #144503

Draft

shmuelhanoch changed the title ~~ESQL: disallow unmapped_fields=load with partial non-KEYWORD~~ Disallow unmapped_fields=load with partial non-KEYWORD Mar 18, 2026

alex-spies added v9.4.0 and removed v7.9.4 labels Mar 18, 2026

alex-spies requested changes Mar 18, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

7c04f1d

…fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

shmuelhanoch added 2 commits March 25, 2026 18:17

shmuelhanoch force-pushed the 143218-esql-unmapped-fields-load branch from 921e12b to a83ca15 Compare March 25, 2026 16:36

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

8d2b3cf

…fields-load

astefan reviewed Mar 26, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

cd3878e

…fields-load

mouhc1ne and others added 2 commits March 26, 2026 10:46

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

f9b0281

…fields-load

alex-spies reviewed Mar 26, 2026

View reviewed changes

mouhc1ne added 4 commits March 26, 2026 12:15

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

b136452

…fields-load

Merge remote-tracking branch 'fork-shmuel/143218-esql-unmapped-fields…

d8ae10a

…-load' into 143218-esql-unmapped-fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

changes below

7233c83

- Move the detection and rejection of partially unmapped non-keywords to Verifier

clean-up

1e1b19a

alex-spies approved these changes Mar 26, 2026

View reviewed changes

mouhc1ne added 2 commits March 26, 2026 15:34

changes below

25f7c1f

- Minor change in how we collect PUNKs: it now avoids string comparisons and relies on es relation output and attribute sets.

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

0a29dc2

…fields-load

mouhc1ne requested a review from alex-spies March 26, 2026 19:36

mouhc1ne and others added 12 commits March 27, 2026 10:37

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

b5d63e5

…fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

b24878e

…fields-load

Fix issues that occurred after merge-conflict

02861aa

[CI] Auto commit changes from spotless

7937027

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

cbb59df

…fields-load

Merge remote-tracking branch 'fork-shmuel/143218-esql-unmapped-fields…

bc86417

…-load' into 143218-esql-unmapped-fields-load # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

cb42da0

…fields-load

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

3e9e2df

…fields-load

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

289449e

…fields-load

changes below:

3298e14

- Minor adjustments to unit test: assert number of problems, and line/col number. - Revert back the Verifier's PUNK detection to the earlier string based approach with field names, instead of the field attributes approach which changed with Gal's PR.

Merge remote-tracking branch 'origin/main' into 143218-esql-unmapped-…

a4cede8

…fields-load

Conversation

shmuelhanoch commented Mar 12, 2026

Uh oh!

elasticsearchmachine commented Mar 16, 2026

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

alex-spies left a comment •

edited

Loading