Skip to content

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Feb 25, 2025

Continuation of #123381

@ivancea ivancea added >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 auto-backport Automatically create backport pull requests when merged v8.19.0 v9.0.1 labels Feb 25, 2025
@ivancea ivancea marked this pull request as ready for review February 25, 2025 17:34
@elasticsearchmachine
Copy link
Collaborator

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) {
Copy link
Member

Choose a reason for hiding this comment

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

foldContext is unused.

Copy link
Contributor Author

@ivancea ivancea Mar 11, 2025

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

Copy link
Member

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.

@ivancea ivancea requested a review from costin March 11, 2025 16:35
ivancea and others added 2 commits March 11, 2025 17:36
…atables

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java
ivancea added a commit that referenced this pull request Mar 11, 2025
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
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Mar 11, 2025
…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
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Mar 11, 2025
…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
elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2025
…#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
elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2025
…#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
Copy link
Contributor

@astefan astefan left a 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()))) + "*";
Copy link
Contributor

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?

ivancea added 2 commits March 12, 2025 13:30
# 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
@ivancea
Copy link
Contributor Author

ivancea commented Mar 12, 2025

@astefan I was thinking a bit about it, and I can see some solutions:

  1. We could add FoldContext to the functions like we add the Configuration.
  2. We could make the config hold a transient context instead of just creating it.
  3. We could swap the Configuration in the functions with a FunctionContext, holding all of that, and potential future things. Just to avoid making more overloads.

In all those cases, we end up with this CircuitBreaker-like FoldContext as part of the plan nodes. That's fine; we do that with the CB in other cases. Just commenting because it's the first time we store something like that in the plan I think

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?

@nik9000
Copy link
Member

nik9000 commented Mar 12, 2025

I don't particularly like adding the FoldContext to the functions themselves - the functions are serialized and we need to attach the context on load. We can do it the same way that Configuration works but it feels too clever.

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 Literal? And if the arguments aren't Literal then it waits for the fold rule to do the fold. That way we don't throw away fold results.

albertzaharovits pushed a commit to albertzaharovits/elasticsearch that referenced this pull request Mar 13, 2025
…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
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Mar 13, 2025
…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
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Mar 14, 2025
…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
@ivancea
Copy link
Contributor Author

ivancea commented Mar 14, 2025

@astefan @nik9000 I've opened a PR (#124894) with the alternate approach of taking the Literals instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.0.2 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants