Skip to content

Conversation

@carlosdelest
Copy link
Member

Closes #136608

Full text functions that take a field as a parameter should allow null as a field parameter. This is necessary as the field may not be present on an index mapping, and the local optimizer replaces it by null.

We allow null as a field in FullTextFunctions, meaning that they are nullable and will be replaced by NULL.

This PR also refactors full text functions that depend on a single field (MATCH, MATCH_PHRASE, KNN) to use a common superclass (SingleFieldFullTextFunction) that contains the common field logic.

@elasticsearchmachine
Copy link
Collaborator

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

@carlosdelest carlosdelest added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Oct 31, 2025
elasticsearchmachine added 5 commits October 31, 2025 11:51
…tions-accept-null' into bugfix/esql-full-text-functions-accept-null
…-functions-accept-null

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java
#	x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java
@@ -300,39 +286,7 @@ public final void writeTo(StreamOutput out) throws IOException {

@Override
protected TypeResolution resolveParams() {
return resolveField().and(resolveQuery())
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactoring - all of these are moved up to SingleFieldFullTextFunction

}

@Override
public boolean foldable() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what changed on this PR besides all the refactoring. When the FTF field is null, the FTF is foldable to NULL, and nullable.

new InferNonNullAggConstraint(),
new ReplaceDateTruncBucketWithRoundTo()
new ReplaceDateTruncBucketWithRoundTo(),
new FoldNull()
Copy link
Member Author

Choose a reason for hiding this comment

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

@alex-spies I added foldNull to local logical optimization, so full text functions can be folded to null when null has been replaced. Does it look OK to you?

Copy link
Contributor

@bpintea bpintea Nov 4, 2025

Choose a reason for hiding this comment

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

I think this should be beneficial, yes. Not sure why we didn't have it earlier.
Do you think you could please add one/some LocalLogicalPlanOptimizerTests with maybe some "easy" other function(s) (string or math?) to check that they locally now get folded too. This should add a tiny bit of performance to other functions too.

Copy link
Member

Choose a reason for hiding this comment

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

the FoldNull rule is in the localOperators batch, that is applied after the localRewrites batch. Is it too late to wait for it to be executed in localOperators? I wonder why we need a duplicate of it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see - I didn't understand that this rule was already used as part of localOperators(). Removing it, thanks @fang-xing-esql !

""");

// Limit[1000[INTEGER],false,false]
Limit limit = as(plan, Limit.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

@fang-xing-esql , now FTFs can be pushed down - when a field does not exist in the index, it will become NULL instead.

Does this make sense from the union all perspective?

@carlosdelest carlosdelest marked this pull request as ready for review October 31, 2025 14:35
@elasticsearchmachine elasticsearchmachine added the Team:Search - Relevance The Search organization Search Relevance team label Oct 31, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-relevance (Team:Search - Relevance)

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but will defer to someone with more ES|QL experience to 👍 😁

public void testFullTextFunctionCannotBePushedDownPastUnionAll() {
assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled());
VerificationException e = expectThrows(VerificationException.class, () -> planSubquery("""
var plan = planSubquery("""
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify a new capability for this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't, as the MATCH function is already released (so it's not conditionally added for release / snapshots) and this test is not executed for BwC purposes.

* - Field verification
* - Serialization patterns
*/
public abstract class SingleFieldFullTextFunction extends FullTextFunction
Copy link
Member

Choose a reason for hiding this comment

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

Nice refactoring!

@carlosdelest
Copy link
Member Author

@elasticsearchmachine run elasticsearch-ci/bwc-snapshots-part3

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

Left a few comments, but after those it should look good.
I think some simple CSV-spec tests would be good to have too, just to check that they work with literals, centrally too. (MATCH(null, ..), ..| EVAL x = null | WHERE MATCH(x, ...)).

Would be great to wait for @fang-xing-esql check too.

Comment on lines 254 to 255
static String expectedTypesAsString(Set<DataType> FieldDataTypes) {
return String.join(", ", FieldDataTypes.stream().map(dt -> dt.name().toLowerCase(Locale.ROOT)).toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static String expectedTypesAsString(Set<DataType> FieldDataTypes) {
return String.join(", ", FieldDataTypes.stream().map(dt -> dt.name().toLowerCase(Locale.ROOT)).toList());
static String expectedTypesAsString(Set<DataType> dataTypes) {
return String.join(", ", dataTypes.stream().map(DataType::typeName).toList());

* Returns a human-readable string listing the expected field types.
* Used in error messages.
*/
protected String getExpectedFieldTypesString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected String getExpectedFieldTypesString() {
protected String expectedFieldTypesString() {

for consistency (with the static method).

* Returns a human-readable string listing the expected query types.
* Used in error messages.
*/
protected String getExpectedQueryTypesString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected String getExpectedQueryTypesString() {
protected String expectedQueryTypesString() {

protected FieldAttribute fieldAsFieldAttribute(Expression field) {
Expression fieldExpression = field;
// Field may be converted to other data type (field_name :: data_type), so we need to check the original field
if (fieldExpression instanceof AbstractConvertFunction convertFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A somewhat lateral note: we could have an unpacking loop here, for the (more likely automated query generation) case of nested conversions, like: field::INTEGER::LONG.

Comment on lines 77 to 81
@Override
protected List<Batch<LogicalPlan>> batches() {
return RULES;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a move, right? Was it intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, reverting

AbstractMatchFullTextFunctionTests.addQueryAsStringTestCases(suppliers);
AbstractMatchFullTextFunctionTests.addStringTestCases(suppliers);
return suppliers;
return addNullFieldTestCases(suppliers);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these aren't added to all functions, like MatchPhraseTests (possibly others, Knn?), right?
Raising this since MatchPhrase.FIELD_DATA_TYPES doesn't include NULL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Done in 845cd35

new InferNonNullAggConstraint(),
new ReplaceDateTruncBucketWithRoundTo()
new ReplaceDateTruncBucketWithRoundTo(),
new FoldNull()
Copy link
Contributor

@bpintea bpintea Nov 4, 2025

Choose a reason for hiding this comment

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

I think this should be beneficial, yes. Not sure why we didn't have it earlier.
Do you think you could please add one/some LocalLogicalPlanOptimizerTests with maybe some "easy" other function(s) (string or math?) to check that they locally now get folded too. This should add a tiny bit of performance to other functions too.

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

Labels

:Analytics/ES|QL AKA ESQL >bug :Search Relevance/ES|QL Search functionality in ES|QL :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search - Relevance The Search organization Search Relevance team Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ESQL: MATCH function failing on locally missing field

5 participants