[ESQL] Greatest and Least can fail with Illegal Data Type on types that resolved correctly #114036
Labels
Comments
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @not-napoleon Can I take this on?:) |
|
Hi @not-napoleon @alex-spies , Is this issue still relevant? If it hasn't been picked up by someone else, I would be happy to work on it. |
|
Hi, are someone currently working on this, I would love to be assigned for this one |
|
Hi @not-napoleon, I am taking on this issue. |
|
Hi, @not-napoleon. @BinhMike. |
shinsj4653
added a commit
to shinsj4653/elasticsearch
that referenced
this issue
May 25, 2025
This commit refactors the Greatest function to use an evaluator map for better code organization. The changes include: - Added EVALUATOR_MAP for evaluator factory mapping - Updated toEvaluator to use map-based lookup - Added NULL type validation - Updated error messages to use getWriteableName() The refactoring improves code readability and maintainability by: - Replacing if-else chains with a map-based lookup - Centralizing evaluator factory creation - Adding consistent NULL type validation - Using getWriteableName() for error messages Relates to elastic#114036
shinsj4653
added a commit
to shinsj4653/elasticsearch
that referenced
this issue
May 25, 2025
This commit refactors the Least function to use an evaluator map for better code organization. The changes include: - Added EVALUATOR_MAP for evaluator factory mapping - Updated toEvaluator to use map-based lookup - Added NULL type validation - Updated error messages to use getWriteableName() The refactoring improves code readability and maintainability by: - Replacing if-else chains with a map-based lookup - Centralizing evaluator factory creation - Adding consistent NULL type validation - Using getWriteableName() for error messages Relates to elastic#114036
shinsj4653
added a commit
to shinsj4653/elasticsearch
that referenced
this issue
May 25, 2025
This commit adds test cases for both Greatest and Least functions to verify: - All supported data types (boolean, double, integer, long, keyword, text, version, ip, datetime, date_nanos) - NULL value handling - String comparisons - Numeric comparisons - Boolean operations - Date/time comparisons The tests ensure that: - Both functions handle NULL values correctly - Type validation works as expected - Evaluator selection is correct for each data type - Results are computed correctly for all supported types Relates to elastic#114036
11 tasks
|
I've created a PR to refactor the Greatest and Least functions to use an evaluator map: #128429 The changes include:
PTAL when you have time. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Related to #113961
resolveTypefor Greatest and Least only checks that all the input data types are the same (or null). ThetoEvaluatorfunction, however, has an explicit list of types it maps to evaluators, and if a type does not match we throw from there. Personally, I recommend a strategy like we used in the binary comparisons, where we have a map from data types to evaluators, and resolve types by checking if they are keys in the map. There are probably other solutions as well. The goal is thatresolveTypeshould be consistent withtoEvaluatorwith regard to type support.The text was updated successfully, but these errors were encountered: