Add semantic_text to mapping_all_types and switch to TranslationAware in PushFiltersToSource #118982
Conversation
…Aware in PushFiltersToSource
ioanatia
added
>non-issue
auto-backport
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| } 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) { |
There was a problem hiding this comment.
- remove the check for
Term- the same way we removed it forMatch. This check is not needed sinceTerm(the same asMatch) implements theValidatableinterface. - use
TranslationAwareinstead ofFullTextFunction. In the initial proposalTranslationAwarehas aboolean translatable()method. We don't have that yet - this is also a check at the data node level and we expect that if aTranslationAwareexpression cannot be pushed down, that would have been caught much earlier at the verifier level.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
TranslationAware is effectively used:
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 🤷♀️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
okay - I can revert this for now - no issue - and we'll come back to it later
| }, | ||
| "semantic_text": { | ||
| "type": "semantic_text", | ||
| "inference_id": "foo_inference_id" |
| } 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) { |
There was a problem hiding this comment.
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.
|
@elasticmachine merge upstream |
| @@ -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()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this is already tested:
💚 Backport successful
|
… 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 <[email protected]>
… 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 <[email protected]>
tracked in #115103
Follow up on a few things that have not been addressed as part of these PRs:
#118355 (comment)
#118676 (comment)