Skip to content
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] Dependency Injection for the Warnings Collector #108635

Closed

Conversation

not-napoleon
Copy link
Member

@not-napoleon not-napoleon commented May 14, 2024

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 boolean to toggle 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.

@not-napoleon not-napoleon removed the WIP label May 15, 2024
@not-napoleon
Copy link
Member Author

@elasticmachine update branch

@not-napoleon
Copy link
Member Author

@elasticmachine update branch

@not-napoleon
Copy link
Member Author

@elasticmachine update branch

@not-napoleon not-napoleon marked this pull request as ready for review May 21, 2024 16:18
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@costin
Copy link
Member

costin commented May 21, 2024

Had a chat with Mark about the logistics behind it - to avoid the noise of having every ExpressionEvaluator.Factory.get() method take a bool, the approach is to put the bool inside DriverContext.
This way the Factory can check what type of warning consumer to create based on the DriverContext instance.
Furthermore in case of folding, in ESQL, this is done in EvaluatorMapper inside the mapper package so the "special" create DriverContext used for folding (with the NON_RECYCLING_INSTANCE) could have be set there.
i.e.

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)
            )

//can become
DriverContext.localDriver(boolean logWarning);

To address the original bug, this logic would be expanded to contain a tryFold() method would not throw warnings OR revisit the folding semantics so that folding does NOT throw warnings (since it can only occur on literals).

@not-napoleon
Copy link
Member Author

Closing in favor of #108927. With the requested changes and the large number of conflicts due to the big refactor, it was faster to just start from scratch.

@alex-spies alex-spies closed this May 23, 2024
not-napoleon added a commit that referenced this pull request Jun 4, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants