[ESQL] Dependency Injection for the Warnings Collector #108927
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
dnhatn
pushed a commit
to dnhatn/elasticsearch
that referenced
this pull request
Jun 4, 2024
This lays the groundwork for fixing elastic#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 elastic#108635
craigtaverner
pushed a commit
to craigtaverner/elasticsearch
that referenced
this pull request
Jun 11, 2024
This lays the groundwork for fixing elastic#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 elastic#108635
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces #108635
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.