Skip to content

ESQL: Fix wrong marking of a field as unmapped when indices shared the same mapping #133298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 27, 2025
Merged

ESQL: Fix wrong marking of a field as unmapped when indices shared the same mapping #133298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1e6f531
ESQL: Fix wrong marking of a field as unmapped when indices shared th…
GalLalouche Aug 21, 2025
61a704a
PR Fixes
GalLalouche Aug 21, 2025
a2a31ee
Merge branch 'main' into gall_bug/fix_insist_same_mapping_20250821
GalLalouche Aug 21, 2025
db78073
Fix charset
GalLalouche Aug 21, 2025
8ac11f8
Merge branch 'main' into gall_bug/fix_insist_same_mapping_20250821
GalLalouche Aug 27, 2025
7d52d22
Merge branch 'main' into gall_bug/fix_insist_same_mapping_20250821
GalLalouche Aug 27, 2025
0a4cce4
Merge branch 'main' into gall_bug/fix_insist_same_mapping_20250821
GalLalouche Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/IndexResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ public void resolveAsMergedMapping(

// public for testing only
public static IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResponse fieldCapsResponse) {
var numberOfIndices = fieldCapsResponse.getIndexResponses().size();
assert ThreadPool.assertCurrentThreadPool(ThreadPool.Names.SEARCH_COORDINATION); // too expensive to run this on a transport worker
if (fieldCapsResponse.getIndexResponses().isEmpty()) {
return IndexResolution.notFound(indexPattern);
Expand Down Expand Up @@ -143,8 +142,7 @@ public static IndexResolution mergedMappings(String indexPattern, FieldCapabilit
);
fields.put(name, field);

var isPartiallyUnmapped = fcs.size() < numberOfIndices;
if (isPartiallyUnmapped) {
if (fieldCapsResponse.getIndexResponses().stream().allMatch(fcir -> fcir.get().containsKey(fullName)) == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this could be a bit expensive for many fields or many indices or both cases.
Lets check if this affects esql/from_idx_limit_1 benchmark after merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed the potential issue by instead of also taking into account the number of mapping hash duplicates.

partiallyUnmappedFields.add(fullName);
}
}
Expand Down
25 changes: 24 additions & 1 deletion x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
import org.elasticsearch.action.fieldcaps.IndexFieldCapabilities;
import org.elasticsearch.action.fieldcaps.IndexFieldCapabilitiesBuilder;
import org.elasticsearch.common.hash.MessageDigests;
import org.elasticsearch.common.lucene.BytesRefs;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexMode;
Expand Down Expand Up @@ -3101,6 +3102,27 @@ public void testResolveInsist_multiIndexFieldPartiallyExistsWithMultiTypesNoKeyw
assertThat(attr.unresolvedMessage(), is(expected));
}

public void testResolveInsist_multiIndexSameMapping_fieldIsMapped() {
assumeTrue("Requires UNMAPPED FIELDS", EsqlCapabilities.Cap.UNMAPPED_FIELDS.isEnabled());

IndexResolution resolution = IndexResolver.mergedMappings(
"foo, bar",
new FieldCapabilitiesResponse(
List.of(
fieldCapabilitiesIndexResponse("foo", messageResponseMap("long")),
fieldCapabilitiesIndexResponse("bar", messageResponseMap("long"))
),
List.of()
)
);
var plan = analyze("FROM foo, bar | INSIST_🐔 message", analyzer(resolution, TEST_VERIFIER));
var limit = as(plan, Limit.class);
var insist = as(limit.child(), Insist.class);
var attribute = (FieldAttribute) EsqlTestUtils.singleValue(insist.output());
assertThat(attribute.name(), is("message"));
assertThat(attribute.dataType(), is(DataType.LONG));
}

public void testResolveInsist_multiIndexFieldPartiallyExistsWithMultiTypesWithKeyword_createsAnInvalidMappedField() {
assumeTrue("Requires UNMAPPED FIELDS", EsqlCapabilities.Cap.UNMAPPED_FIELDS.isEnabled());

Expand Down Expand Up @@ -3531,7 +3553,8 @@ private static FieldCapabilitiesIndexResponse fieldCapabilitiesIndexResponse(
String indexName,
Map<String, IndexFieldCapabilities> fields
) {
return new FieldCapabilitiesIndexResponse(indexName, indexName, fields, false, IndexMode.STANDARD);
String indexMappingHash = new String(MessageDigests.sha256().digest(fields.toString().getBytes()));
return new FieldCapabilitiesIndexResponse(indexName, indexMappingHash, fields, false, IndexMode.STANDARD);
}

private static Map<String, IndexFieldCapabilities> messageResponseMap(String date) {
Expand Down