-
Notifications
You must be signed in to change notification settings - Fork 25k
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
ESQL: Add DriverContext to the construction of Evaluators #99518
Changes from all commits
30d9dee
bbc94c6
0581b8c
fe3947c
68dcc09
eb20d9e
bf70c0b
f10d1ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,12 @@ | |
|
||
package org.elasticsearch.compute.operator; | ||
|
||
import org.elasticsearch.common.util.BigArrays; | ||
import org.elasticsearch.core.Releasable; | ||
|
||
import java.util.Collections; | ||
import java.util.IdentityHashMap; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
|
||
|
@@ -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); | ||
|
||
// Working set. Only the thread executing the driver will update this set. | ||
Set<Releasable> workingSet = Collections.newSetFromMap(new IdentityHashMap<>()); | ||
|
||
private final AtomicReference<Snapshot> snapshot = new AtomicReference<>(); | ||
|
||
private final BigArrays bigArrays; | ||
|
||
// For testing | ||
public DriverContext() { | ||
this(BigArrays.NON_RECYCLING_INSTANCE); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
public DriverContext(BigArrays bigArrays) { | ||
Objects.requireNonNull(bigArrays); | ||
this.bigArrays = bigArrays; | ||
} | ||
|
||
public BigArrays bigArrays() { | ||
return bigArrays; | ||
} | ||
|
||
/** A snapshot of the driver context. */ | ||
public record Snapshot(Set<Releasable> releasables) {} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,24 +10,22 @@ | |
import org.elasticsearch.compute.data.Block; | ||
import org.elasticsearch.compute.data.Page; | ||
|
||
import java.util.function.Supplier; | ||
|
||
/** | ||
* Evaluates a tree of functions for every position in the block, resulting in a | ||
* new block which is appended to the page. | ||
*/ | ||
public class EvalOperator extends AbstractPageMappingOperator { | ||
|
||
public record EvalOperatorFactory(Supplier<ExpressionEvaluator> evaluator) implements OperatorFactory { | ||
public record EvalOperatorFactory(ExpressionEvaluator.Factory evaluator) implements OperatorFactory { | ||
|
||
@Override | ||
public Operator get(DriverContext driverContext) { | ||
return new EvalOperator(evaluator.get()); | ||
return new EvalOperator(evaluator.get(driverContext)); | ||
} | ||
|
||
@Override | ||
public String describe() { | ||
return "EvalOperator[evaluator=" + evaluator.get() + "]"; | ||
return "EvalOperator[evaluator=" + evaluator.get(DriverContext.DEFAULT) + "]"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
|
@@ -48,6 +46,12 @@ public String toString() { | |
} | ||
|
||
public interface ExpressionEvaluator { | ||
|
||
/** A Factory for creating ExpressionEvaluators. */ | ||
interface Factory { | ||
ExpressionEvaluator get(DriverContext driverContext); | ||
ChrisHegarty marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
Block eval(Page page); | ||
} | ||
|
||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, like, I guess a "temporary" kind of thing.