-
Notifications
You must be signed in to change notification settings - Fork 25.2k
ESQL: Pass Fold context to translatable expressions #123398
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.
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
base: main
Are you sure you want to change the base?
ESQL: Pass Fold context to translatable expressions #123398
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
…atables # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeComputeHandler.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/planner/FilterTests.java
} | ||
|
||
@Override | ||
public Query asQuery(TranslatorHandler handler, FoldContext foldContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foldContext is unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's part of the interface//Override now, as some functions need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment from Andrei.
…atables # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
Fixes #123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in #123398, which I kept separated as it changes many files
…123381) Fixes elastic#123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in elastic#123398, which I kept separated as it changes many files
…123381) Fixes elastic#123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in elastic#123398, which I kept separated as it changes many files
…#124583) Fixes #123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in #123398, which I kept separated as it changes many files
…#124582) Fixes #123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in #123398, which I kept separated as it changes many files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not contest the implementation of the FoldContext
idea and how it's being passed around to various functions that need it, but I think it would be beneficial, more elegant and less intrusive throughout the code if we can integrate the FoldContext
into the existent infrastructure in a more seamless way. The FoldContext
is being passed around in a lot of methods as arguments.
What triggered my curiosity was the fact that the FoldContext
is created by a method attached to the Configuration
class, which makes it convenient as it's close to the pragma location, but does it actually have a conceptual connection to the Configuration
? Can it be made a conceptual element of the Configuration
?
This is probably related to the question "What is the lifecycle of a FoldContext
? Is the life of a FoldContext
the same as the one of the Configuration
instance itself? Is the life of a FoldContext
the same as the life of a Function instance, as defined by the EsqlFunctionRegistry
?".
Could you, please, explore these questions and ideas?
We have a precedence in *QL in the form of EsqlFunctionRegistry.ConfigurationAwareBuilder
interface, meaning those functions that need something from the Configuration
instance (now
function is the only one so far) they have a constructor that injects the configuration into them and they can do whatever they want with it. Since the folding aspect is one tied to a Function, do we actually need the FoldContext
that comes from "outside" (in the form of arguments passed to functions - fold
being one), or is it possible to use a FoldContext
that belongs to a Function instance (and automatically injected through Configuration
by the EsqlFunctionRegistry
or through another/new interface in EsqlFunctionRegistry
) and not pass the context over and over throughout the code?
public Query asQuery(TranslatorHandler handler, FoldContext foldContext) { | ||
LucenePushdownPredicates.checkIsPushableAttribute(str); | ||
var fieldName = handler.nameOf(str instanceof FieldAttribute fa ? fa.exactAttribute() : str); | ||
var wildcardQuery = QueryParser.escape(BytesRefs.toString(prefix.fold(FoldContext.small()))) + "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are still using FoldContext.small()
.... Shouldn't this use the foldContext
instead?
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/EndsWith.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/StartsWith.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/EndsWithTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/StartsWithTests.java
@astefan I was thinking a bit about it, and I can see some solutions:
In all those cases, we end up with this I would go with either 2 (store in Configuration) or 3 (new context in functions). Probably 3, as random transient fields feel weird to me if we can avoid them. @nik9000, any thoughts around this? |
I don't particularly like adding the I suppose I like passing it around because it makes us ask "should this fold?" Like, should translation fold? Or should it cast to a |
…123381) Fixes elastic#123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in elastic#123398, which I kept separated as it changes many files
…123381) Fixes elastic#123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in elastic#123398, which I kept separated as it changes many files
…123381) Fixes elastic#123067 Just like WildcardLike and RLike, some functions can be converted to Lucene queries. Here it's those two, which are nearly identical to WildcardLike This, like some other functions, needs a FoldContext. I'm using the static method for this here, but it's fixed in elastic#123398, which I kept separated as it changes many files
Continuation of #123381