Add semantic_text to mapping_all_types and switch to TranslationAware in PushFiltersToSource by ioanatia · Pull Request #118982 · elastic/elasticsearch

ioanatia · 2024-12-18T15:56:03Z

tracked in #115103

Follow up on a few things that have not been addressed as part of these PRs:

#118355 (comment)
#118676 (comment)

…Aware in PushFiltersToSource

elasticsearchmachine · 2024-12-18T15:56:46Z

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

ioanatia · 2024-12-18T16:20:04Z

...in/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java

-        } else if (exp instanceof Term term) {
-            return term.field() instanceof FieldAttribute && DataType.isString(term.field().dataType());
-        } else if (exp instanceof FullTextFunction) {
+        } else if (exp instanceof TranslationAware) {


remove the check for Term - the same way we removed it for Match. This check is not needed since Term (the same as Match) implements the Validatable interface.

use TranslationAware instead of FullTextFunction. In the initial proposal TranslationAware has a boolean translatable() method. We don't have that yet - this is also a check at the data node level and we expect that if a TranslationAware expression cannot be pushed down, that would have been caught much earlier at the verifier level.

If we're not using yet TranslationAware directly in the codebase, why make this change? 🤔

For me it's good to keep using FullTextFunction until TranslationAware is effectively used.

TranslationAware is effectively used:

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

Line 36 in 24773e0

public abstract class FullTextFunction extends Function implements TranslationAware {

what we don't have is that boolean translatable() method and I think for now we don't need it, but we might in the future 🤷‍♀️

what we don't have is that boolean translatable() method and I think for now we don't need it, but we might in the future 🤷‍♀️

sorry, that's what I meant - we just have the interface as a marker and the interface method is not actually used. I don't see the advantage on making the switch until the interface method is actually used.

okay - I can revert this for now - no issue - and we'll come back to it later

carlosdelest · 2024-12-18T16:37:55Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/mapping-all-types.json

+        },
+        "semantic_text": {
+            "type": "semantic_text",
+            "inference_id": "foo_inference_id"


carlosdelest · 2024-12-18T16:41:57Z

...in/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java

-        } else if (exp instanceof Term term) {
-            return term.field() instanceof FieldAttribute && DataType.isString(term.field().dataType());
-        } else if (exp instanceof FullTextFunction) {
+        } else if (exp instanceof TranslationAware) {


If we're not using yet TranslationAware directly in the codebase, why make this change? 🤔

For me it's good to keep using FullTextFunction until TranslationAware is effectively used.

ioanatia · 2024-12-19T14:31:04Z

@elasticmachine merge upstream

luigidellaquila

LGTM, thanks!

luigidellaquila · 2024-12-30T07:58:00Z

...in/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java

        } 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());


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.

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)

)

);

}

…esql_follow_up

elasticsearchmachine · 2025-01-07T11:20:34Z

💚 Backport successful

Status	Branch	Result
✅	8.x

… in PushFiltersToSource (elastic#118982) * Add semantic_text to mapping-all-types.json and switch to TranslationAware in PushFiltersToSource * Continue to use FullTextFunction for now --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

… in PushFiltersToSource (#118982) (#119635) * Add semantic_text to mapping-all-types.json and switch to TranslationAware in PushFiltersToSource * Continue to use FullTextFunction for now --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Add semantic_text to mapping-all-types.json and switch to Translation…

683a843

…Aware in PushFiltersToSource

ioanatia added >non-issue auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.0 labels Dec 18, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 18, 2024

elasticsearchmachine added the v9.0.0 label Dec 18, 2024

ioanatia commented Dec 18, 2024

View reviewed changes

ioanatia requested review from carlosdelest and luigidellaquila December 18, 2024 16:20

carlosdelest reviewed Dec 18, 2024

View reviewed changes

Continue to use FullTextFunction for now

b3c6f7c

Merge branch 'main' into semantic_text_esql_follow_up

9dc94fc

luigidellaquila approved these changes Dec 30, 2024

View reviewed changes

Merge remote-tracking branch 'elasticsearch/main' into semantic_text_…

ced0519

…esql_follow_up

ioanatia merged commit 355aed7 into elastic:main Jan 7, 2025

This was referenced Jan 7, 2025

Support semantic_text in ES|QL #115103

Closed

[8.x] Add semantic_text to mapping_all_types and switch to TranslationAware in PushFiltersToSource (#118982) #119635

Merged

	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)
	)
	);
	}

Conversation

ioanatia commented Dec 18, 2024

Uh oh!

elasticsearchmachine commented Dec 18, 2024

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!

ioanatia commented Dec 19, 2024

Uh oh!

luigidellaquila left a comment

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!

elasticsearchmachine commented Jan 7, 2025

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants