Skip to content
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

Add semantic_text to mapping_all_types and switch to TranslationAware in PushFiltersToSource #118982

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Add semantic_text to mapping_all_types and switch to TranslationAware in PushFiltersToSource #118982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
683a843
Add semantic_text to mapping-all-types.json and switch to Translation…
ioanatia Dec 18, 2024
b3c6f7c
Continue to use FullTextFunction for now
ioanatia Dec 19, 2024
9dc94fc
Merge branch 'main' into semantic_text_esql_follow_up
elasticmachine Dec 19, 2024
ced0519
Merge remote-tracking branch 'elasticsearch/main' into semantic_text_…
ioanatia Jan 7, 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: 4 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/mapping-all-types.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@
},
"wildcard": {
"type": "wildcard"
},
"semantic_text": {
"type": "semantic_text",
"inference_id": "foo_inference_id"
Copy link
Member

Choose a reason for hiding this comment

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

❤️

}
}
}
3 changes: 0 additions & 3 deletions ...java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
import org.elasticsearch.xpack.esql.core.util.Queries;
import org.elasticsearch.xpack.esql.expression.function.fulltext.FullTextFunction;
import org.elasticsearch.xpack.esql.expression.function.fulltext.Term;
import org.elasticsearch.xpack.esql.expression.function.scalar.ip.CIDRMatch;
import org.elasticsearch.xpack.esql.expression.function.scalar.spatial.BinarySpatialFunction;
import org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesFunction;
Expand Down Expand Up @@ -252,8 +251,6 @@ static boolean canPushToSource(Expression exp, LucenePushdownPredicates lucenePu
&& Expressions.foldable(cidrMatch.matches());
} else if (exp instanceof SpatialRelatesFunction spatial) {
return canPushSpatialFunctionToSource(spatial, lucenePushdownPredicates);
} else if (exp instanceof Term term) {
return term.field() instanceof FieldAttribute && DataType.isString(term.field().dataType());
Copy link
Contributor

Choose a reason for hiding this comment

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

This was redundant indeed, I tried it quickly and it works fine (the verifier prevents these cases).

nit: If you have a chance, it would be good to have a VerifierTest that checks that things like WHERE term("foo", "bar") return the right verification exception and don't result in a wrong pushdown.

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 think this is already tested:

elasticsearch/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/fulltext/TermTests.java

Lines 86 to 99 in 9424bd2

private static void addNonFieldTestCase(
List<DataType> paramDataTypes,
List<Set<DataType>> supportedPerPosition,
List<TestCaseSupplier> suppliers
) {
// Negative case - use directly the field parameter type
suppliers.add(
new TestCaseSupplier(
getTestCaseName(paramDataTypes, "-non ES field"),
paramDataTypes,
typeErrorSupplier(true, supportedPerPosition, paramDataTypes, TermTests::matchTypeErrorSupplier)
)
);
}

} else if (exp instanceof FullTextFunction) {
return true;
}
Expand Down
5 changes: 0 additions & 5 deletions ...src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,11 +1450,6 @@ private void checkMatchFunctionPushDown(
var analyzer = makeAnalyzer("mapping-all-types.json");
// Check for every possible query data type
for (DataType fieldDataType : fieldDataTypes) {
// TODO: semantic_text is not present in mapping-all-types.json so we skip it for now
if (fieldDataType == DataType.SEMANTIC_TEXT) {
continue;
}

var queryValue = randomQueryValue(fieldDataType);

String fieldName = fieldDataType == DataType.DATETIME ? "date" : fieldDataType.name().toLowerCase(Locale.ROOT);
Expand Down