Skip to content

ESQL: Type conflict resolution in unmapped-fields load#143693

Open
GalLalouche wants to merge 164 commits intoelastic:mainfrom
GalLalouche:type_conflicts
Open

ESQL: Type conflict resolution in unmapped-fields load#143693
GalLalouche wants to merge 164 commits intoelastic:mainfrom
GalLalouche:type_conflicts

Conversation

@GalLalouche
Copy link
Contributor

@GalLalouche GalLalouche commented Mar 5, 2026

Resolves #141912.
Resolves #142004.

GalLalouche and others added 30 commits January 21, 2026 23:04
PotentiallyUnmappedKeywordEsField (used for fields loaded from _source
when unmapped_fields="load") had isAggregatable=true, which caused
isPushableFieldAttribute to short-circuit past the SearchStats check and
push filters down to Lucene. On shards where the field is not indexed,
the Lucene query returns no results instead of letting the compute
engine evaluate the filter on _source-loaded values.

Guard isPushableFieldAttribute against PotentiallyUnmappedKeywordEsField
so these fields are always filtered in the compute engine.

Co-authored-by: Cursor <cursoragent@cursor.com>
The isPushableFieldAttribute fix (rejecting PotentiallyUnmappedKeywordEsField)
already covers sort pushdown since PushTopNToSource uses the same method.
This test verifies correct sort order when a field is mapped in one index
but unmapped in another under unmapped_fields="load".

Co-authored-by: Cursor <cursoragent@cursor.com>
@elasticsearchmachine
Copy link
Collaborator

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

1 similar comment
@elasticsearchmachine
Copy link
Collaborator

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

;

partiallyUnmappedNonKeyword
required_capability: optional_fields_v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped.

required_capability: index_metadata_field

SET unmapped_fields="load"\;
FROM sample_data, no_mapping_sample_data METADATA _index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

;

#######################
# Type conflict tests #
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

# Type conflict tests #
#######################

typeConflictKeywordUnmappedCastToKeyword
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

var convert = new ToDateNanos(f.source(), f, context.configuration());
imf.types().forEach(type -> typeResolutions(f, convert, type, imf, typeResolutions));
var resolvedField = ResolveUnionTypes.resolvedMultiTypeEsField(f, typeResolutions);
var resolvedField = ResolveUnionTypes.resolvedMultiTypeEsField(f, typeResolutions, null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So while analyzing this, I uncovered an underlying pre-existing union type bug! #144870.

It looks like this is actually fine to be null (I added an assertion), but the other one isn't.

indexToConversionExpressions,
fa.field().getTimeSeriesFieldType()
fa.field().getTimeSeriesFieldType(),
null // FIXME(gal, NOCOMMIT) This basically a copy constructor, shouldn't be null; write a test to flush it out.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually fine, due to the behavior mentioned in #144870. I've added an assertion and comment instead.

@@ -2633,7 +2653,7 @@ public LogicalPlan apply(LogicalPlan plan, AnalyzerContext context) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part though, is a bug! If some fields are unmapped and load is on, then not all types are actually dates! Fixed and added tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, except TS. Now it's a race to see who finishes first :)

indexToConversionExpressions,
fa.field().getTimeSeriesFieldType()
fa.field().getTimeSeriesFieldType(),
null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an assert and a comment why this should be null.

if (indexMappingHashToDuplicateCount.compute(response.getIndexMappingHash(), (k, v) -> v == null ? 1 : v + 1) > 1) {
continue;
}
boolean isNew = indexMappingHashToDuplicateCount.compute(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug found by existing INSIST_🐔 tests! (The one I stupidly deleted because I didn't want to start fixing INSIST code 🤦.) If two indices share the same mapping, we still need to update fieldToMappedIndices. I've transferred the tests to use unmapped_fields=load instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >feature Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.4.0

Projects

None yet