Skip to content

Commit

Permalink
[ESQL] Dependency Injection for the Warnings Collector (#108927)
Browse files Browse the repository at this point in the history
This lays the groundwork for fixing #108519, although I don't intend to fix it in this PR. In order to address that issue, we need to be able to control at call time how we deal with warnings, but, prior to this work, the ExpressionEvaluators all hard code their warnings implementation.

This PR will allow injecting in a different implementation when we create the evaluator. Unfortunately, this has fallen slightly short of that ambition. We need to be able to construct evaluators from within compute, but the Warnings class, and it's Source dependency, are in the esql package, which we don't want to import into compute. As result, I've pared this back to just passing in a toggle for warnings collection, via a no-op collector implementation. This is sufficient to solve the linked issue (still not done in this PR) and preserves our current encapsulation, while leaving a clear path to further refine how we control warnings in the future.

Note that this is changing the code generation, which means it will touch all of the generated files, which are still in source control. The actual change is much smaller than the diff would imply.

Replaces #108635
  • Loading branch information
not-napoleon committed Jun 4, 2024
1 parent 8ac3e3d commit c2e3505
Show file tree
Hide file tree
Showing 190 changed files with 247 additions and 199 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,11 @@ private TypeSpec type() {
private MethodSpec ctor() {
MethodSpec.Builder builder = MethodSpec.constructorBuilder().addModifiers(Modifier.PUBLIC);
builder.addParameter(SOURCE, "source");
builder.addStatement("this.warnings = new Warnings(source)");
processFunction.args.stream().forEach(a -> a.implementCtor(builder));

builder.addParameter(DRIVER_CONTEXT, "driverContext");
builder.addStatement("this.driverContext = driverContext");
builder.addStatement("this.warnings = Warnings.createWarnings(driverContext.warningsMode(), source)");
return builder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,10 @@ private MethodSpec ctor() {
}
builder.addParameter(EXPRESSION_EVALUATOR, "field");
builder.addStatement("super(driverContext, field)");
builder.addParameter(DRIVER_CONTEXT, "driverContext");
if (warnExceptions.isEmpty() == false) {
builder.addStatement("this.warnings = new Warnings(source)");
builder.addStatement("this.warnings = Warnings.createWarnings(driverContext.warningsMode(), source)");
}
builder.addParameter(DRIVER_CONTEXT, "driverContext");
return builder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.SubscribableListener;
import org.elasticsearch.common.breaker.CircuitBreaker;
import org.elasticsearch.common.breaker.NoopCircuitBreaker;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.compute.data.BlockFactory;
import org.elasticsearch.core.Releasable;
Expand Down Expand Up @@ -57,11 +58,26 @@ public class DriverContext {

private final AsyncActions asyncActions = new AsyncActions();

private final WarningsMode warningsMode;

public DriverContext(BigArrays bigArrays, BlockFactory blockFactory) {
this(bigArrays, blockFactory, WarningsMode.COLLECT);
}

private DriverContext(BigArrays bigArrays, BlockFactory blockFactory, WarningsMode warningsMode) {
Objects.requireNonNull(bigArrays);
Objects.requireNonNull(blockFactory);
this.bigArrays = bigArrays;
this.blockFactory = blockFactory;
this.warningsMode = warningsMode;
}

public static DriverContext getLocalDriver() {
return new DriverContext(
BigArrays.NON_RECYCLING_INSTANCE,
// TODO maybe this should have a small fixed limit?
new BlockFactory(new NoopCircuitBreaker(CircuitBreaker.REQUEST), BigArrays.NON_RECYCLING_INSTANCE)
);
}

public BigArrays bigArrays() {
Expand Down Expand Up @@ -159,6 +175,22 @@ public void removeAsyncAction() {
asyncActions.removeInstance();
}

/**
* Evaluators should use this function to decide their warning behavior.
* @return an appropriate {@link WarningsMode}
*/
public WarningsMode warningsMode() {
return warningsMode;
}

/**
* Indicates the behavior Evaluators of this context should use for reporting warnings
*/
public enum WarningsMode {
COLLECT,
IGNORE
}

private static class AsyncActions {
private final SubscribableListener<Void> completion = new SubscribableListener<>();
private final AtomicBoolean finished = new AtomicBoolean();
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit c2e3505

Please sign in to comment.