ESQL: Add DriverContext to the construction of Evaluators #99518
Conversation
ChrisHegarty
added
>refactoring
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
ChrisHegarty
changed the title
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
|
An explicit Evaluator implementations also now take an additional |
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/EvalOperator.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/EvalOperator.java
Show resolved
Hide resolved
...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/evaluator/mapper/EvaluatorMapper.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/execution/PlanExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/execution/PlanExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/execution/PlanExecutor.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/execution/PlanExecutor.java
Outdated
Show resolved
Hide resolved
...ticsearch/xpack/esql/expression/predicate/operator/arithmetic/SubUnsignedLongsEvaluator.java
Outdated
Show resolved
Hide resolved
|
I believe that all feedback has been addressed. |
| // For testing | ||
| public DriverContext() { | ||
| this(BigArrays.NON_RECYCLING_INSTANCE); | ||
| } |
There was a problem hiding this comment.
I know it's a pain, but I'd prefer not to have this in our production code. It'd be super ok on some test class, but it's too tempting.
| @@ -33,11 +35,30 @@ | |||
| */ | |||
| public class DriverContext { | |||
|
|
|||
| /** A default driver context. The returned bigArrays is non recycling. */ | |||
| public static DriverContext DEFAULT = new DriverContext(BigArrays.NON_RECYCLING_INSTANCE); | |||
There was a problem hiding this comment.
It's not really a "default" - it's more of a "don't use this in production" kind of thing.
There was a problem hiding this comment.
Or, like, I guess a "temporary" kind of thing.
| } | ||
|
|
||
| @Override | ||
| public String describe() { | ||
| return "EvalOperator[evaluator=" + evaluator.get() + "]"; | ||
| return "EvalOperator[evaluator=" + evaluator.get(DriverContext.DEFAULT) + "]"; |
There was a problem hiding this comment.
If we're generating factory could we generate the toString in it so we don't have to pass the driver context? I just want to make sure we don't accidentally do stuff with the context.
|
@nik9000 If it's ok, I'll merge this PR as-is, to avoid any conflicts as it touches many places. I'll follow up with the outstanding comments immediately. |
no rush. It's not going to hurt anything. |
This commit adds DriverContext to the construction of Evaluators.
DriverContext is enriched to carry bigArrays, and will eventually carry a BlockFactory - it's the context for code requiring to create instances of blocks and big arrays.
relates #99173