We created a shared data structure (e.g., a map of supported evaluato… #127097
Conversation
…rs by data type) that is referenced consistently in both resolveType and toEvaluator
|
❌ Author of the following commits did not sign a Contributor Agreement: Please, read and sign the above mentioned agreement if you want to contribute to this project |
elasticsearchmachine
added
needs:triage
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
removed
the
needs:triage
There was a problem hiding this comment.
Thanks for your contribution.
This mismatch leads to cases where resolveType succeeds, but toEvaluator fails with an illegal type exception.
Part of adding support for a type is adding evaluators for it. Everything else that trickles through is a defect. Adding a generic evaluator high in the class hierarchy isn't possible.
The existing implementation is correct.
This proposed change isn't correct and lacks to showcase the error in tests it corrects -- you'll need to add tests for nearly every proposed change. You may refer to similar PRs and code base to learn more about which tests would be necessary.
You'll want to give your PRs a correct title too, to summarise their intention.
There was a problem hiding this comment.
These are cosmetic changes unrelated to the proposed change.
There was a problem hiding this comment.
The changes here are incorrect, the evaluators are expressions-specific.
Also, the style doesn't conform to project's -- please refer to https://github.com/elastic/elasticsearch/blob/main/CONTRIBUTING.md
There was a problem hiding this comment.
These are changes that aren't related to the proposed change.
Also, the {} markers are later handled by LoggerMessageFormat#format, so there's no error.
In the existing implementation of Greatest and Least, the resolveType method only checks whether all input data types are the same. However, the toEvaluator function separately maintains a list of types it can handle. This mismatch leads to cases where resolveType succeeds, but toEvaluator fails with an illegal type exception.
Implementation Summary: We created a shared data structure (e.g., a map of supported evaluators by data type) that is referenced consistently in both resolveType and toEvaluator. This approach mirrors the strategy used in binary comparison operators, ensuring type validation is unified. The implementation required modifying the ESQL expression evaluator logic and testing the affected classes for data type coverage.