[9.0] ESQL: Refactor Greatest and Least functions to use evaluator map (#114036) #128429

shinsj4653 · 2025-05-25T16:39:04Z

ESQL: Refactor Greatest and Least functions to use switch statements

Description

This PR refactors the Greatest and Least functions to use Java switch expressions instead of Map-based evaluator lookup for better performance and code clarity. The changes include:

Replaced EVALUATOR_MAP with switch expressions in toEvaluator method
Added isSupportedDataType helper method using switch expression
Improved error messages with proper type validation
Added comprehensive type validation tests in VerifierTests

Changes

Greatest.java:
- Replaced Map-based evaluator lookup with switch expression
- Added isSupportedDataType method for type validation
- Updated toEvaluator to use switch-based factory creation
- Improved error handling and type checking
Least.java:
- Replaced Map-based evaluator lookup with switch expression
- Added isSupportedDataType method for type validation
- Updated toEvaluator to use switch-based factory creation
- Improved error handling and type checking
VerifierTests.java:
- Added testGreatestLeastWithUnsupportedDataTypes() test method
- Tests unsupported data types (geo_point, cartesian_point)
- Tests mixed type scenarios with proper error message validation
- Ensures type consistency between resolveType and toEvaluator

Testing

GreatestTests.java, LeastTests.java

./gradlew :x-pack:plugin:esql:test --tests "*GreatestTests*" --tests "*LeastTests*" -x :libs:simdvec:compileMain21Java -x :libs:simdvec:compileMain22Java --console=plain

VerifierTests.testGreatestLeastWithUnsupportedDataTypes

./gradlew :x-pack:plugin:esql:test --tests "VerifierTests.testGreatestLeastWithUnsupportedDataTypes" -x :libs:simdvec:compileMain21Java -x :libs:simdvec:compileMain22Java --console=plain --info

✅ Greatest/Least Function Tests

GreatestTests: All tests PASSED
LeastTests: All tests PASSED
Covers all supported data types and edge cases
No regressions introduced

✅ VerifierTests - Type Validation

testGreatestLeastWithUnsupportedDataTypes: PASSED
Tests unsupported data types (geo_point, cartesian_point)
Validates proper error messages for type mismatches
Ensures consistency between resolveType and toEvaluator

🔍 Error Scenario Validation

The new type validation correctly rejects unsupported data types:

geo_point → "Cannot use [geo_point] with function [Greatest]"
cartesian_point → "Cannot use [cartesian_point] with function [Least]"
Mixed types → Proper type mismatch error messages

Note: The error messages are validated by the test framework and don't appear in logs when tests pass, which is the expected behavior.

Related Issues

Closes #114036

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Backport

Backport to 8.19
Backport to 8.18
Backport to 8.17

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

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

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

elasticsearchmachine · 2025-06-02T15:22:57Z

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

idegtiarenko · 2025-06-03T06:14:38Z

...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Least.java

+            throw EsqlIllegalArgumentException.illegalDataType(dataType);
+        }
+
+        return evaluatorFactory.apply(factories);


I wonder if this is one of the things worth doing with java`s switch? Each data type is enum and compile time constant

Thanks for the feedback! This is really helpful. I'll work on refactoring the code based on your suggestions :)

alex-spies

Hi @shinsj4653 and thank you very much for your contribution!

I'm sorry it took us so long to get back to you.

Unfortunately, this PR does not contain tests that demonstrate that the earlier type check actually happens and returns the expected error message. We often add cases to the VerifierTests in such situations.

shinsj4653 · 2025-09-26T14:09:41Z

Hi @alex-spies, I appreciate your feedback. I'll add a test case for this issue using VerifierTests

…- Replace Map-based evaluator lookup with Java switch expressions- Add isSupportedDataType helper method for type validation- Improve error handling and type checking consistency- Ensure resolveType and toEvaluator are aligned for type support

… testGreatestLeastWithUnsupportedDataTypes test method- Test unsupported data types (geo_point, cartesian_point)- Test mixed type scenarios with proper error message validation- Ensure type consistency between resolveType and toEvaluator- Validate error messages for type mismatches

shinsj4653 · 2025-09-27T06:59:24Z

Hi @alex-spies @idegtiarenko,

Thank you for the valuable feedback! I've implemented the suggested changes:

✅ idegtiarenko's feedback: Refactored to use switch expressions instead of Map-based evaluator lookup
✅ alex-spies's feedback: Added comprehensive type validation tests in VerifierTests

The changes ensure:

Better performance with switch expressions
Proper type validation for unsupported data types
Consistency between resolveType and toEvaluator

All tests are passing. PTAL and I'd appreciate your review! 🙏

shinsj4653 added 3 commits May 26, 2025 01:32

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels May 25, 2025

github-actions bot deployed to docs-preview May 25, 2025 16:39 View deployment

Fix PR number in commit messages (elastic#128429)

23a28c2

github-actions bot deployed to docs-preview May 25, 2025 16:42 View deployment

shinsj4653 mentioned this pull request May 25, 2025

[ESQL] Greatest and Least can fail with Illegal Data Type on types that resolved correctly #114036

Open

Update docs/changelog/128429.yaml

6e6f92c

github-actions bot deployed to docs-preview May 25, 2025 16:49 View deployment

alex-spies added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Jun 2, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 2, 2025

idegtiarenko reviewed Jun 3, 2025

View reviewed changes

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

alex-spies requested changes Sep 9, 2025

View reviewed changes

bpintea self-assigned this Sep 9, 2025

bpintea added >bug v8.18.7 v8.19.4 auto-backport Automatically create backport pull requests when merged labels Sep 9, 2025

elasticsearchmachine added v8.18.8 v8.19.5 and removed v8.18.7 v8.19.4 labels Sep 11, 2025

shinsj4653 added 2 commits September 27, 2025 15:49

shinsj4653 requested review from alex-spies and idegtiarenko September 27, 2025 06:59

elasticsearchmachine added v8.18.9 v8.19.6 v9.3.0 and removed v8.18.8 v8.19.5 v9.2.0 labels Oct 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[9.0] ESQL: Refactor Greatest and Least functions to use evaluator map (#114036) #128429

[9.0] ESQL: Refactor Greatest and Least functions to use evaluator map (#114036) #128429

Uh oh!

shinsj4653 commented May 25, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented Jun 2, 2025

Uh oh!

idegtiarenko Jun 3, 2025

Uh oh!

shinsj4653 Sep 26, 2025

Uh oh!

alex-spies left a comment

Uh oh!

shinsj4653 commented Sep 26, 2025

Uh oh!

shinsj4653 commented Sep 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[9.0] ESQL: Refactor Greatest and Least functions to use evaluator map (#114036) #128429

Are you sure you want to change the base?

[9.0] ESQL: Refactor Greatest and Least functions to use evaluator map (#114036) #128429

Uh oh!

Conversation

shinsj4653 commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESQL: Refactor Greatest and Least functions to use switch statements

Description

Changes

Testing

✅ Greatest/Least Function Tests

✅ VerifierTests - Type Validation

🔍 Error Scenario Validation

Related Issues

Checklist

Backport

Uh oh!

elasticsearchmachine commented Jun 2, 2025

Uh oh!

idegtiarenko Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

shinsj4653 Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

shinsj4653 commented Sep 26, 2025

Uh oh!

shinsj4653 commented Sep 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shinsj4653 commented May 25, 2025 •

edited

Loading