Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
95f84d2
Refactor Greatest function to use evaluator map
shinsj4653 May 25, 2025
866d650
Refactor Least function to use evaluator map
shinsj4653 May 25, 2025
6f8614f
Add tests for Greatest and Least functions
shinsj4653 May 25, 2025
23a28c2
Fix PR number in commit messages (#128429)
shinsj4653 May 25, 2025
6e6f92c
Update docs/changelog/128429.yaml
shinsj4653 May 25, 2025
5299b22
ESQL: Refactor Greatest and Least functions to use switch expressions…
shinsj4653 Sep 27, 2025
ff3dd9c
ESQL: Add type validation tests for Greatest and Least functions- Add…
shinsj4653 Sep 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/128429.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 128429
summary: [ESQL] Refactor Greatest and Least functions to use evaluator map
area: ES|QL
type: bug
issues:
- 114036
49 changes: 35 additions & 14 deletions ...in/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Greatest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.compute.ann.Evaluator;
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Expression;
Expand All @@ -27,9 +28,15 @@
import org.elasticsearch.xpack.esql.expression.function.scalar.EsqlScalarFunction;
import org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvMax;
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Stream;

import static org.elasticsearch.xpack.esql.core.type.DataType.NULL;
Expand All @@ -42,6 +49,17 @@ public class Greatest extends EsqlScalarFunction implements OptionalArgument {

private DataType dataType;

private static final Map<DataType, Function<ExpressionEvaluator.Factory[], ExpressionEvaluator.Factory>> EVALUATOR_MAP = Map.of(
DataType.BOOLEAN, factories -> new GreatestBooleanEvaluator.Factory(Source.EMPTY, factories),
DataType.DOUBLE, factories -> new GreatestDoubleEvaluator.Factory(Source.EMPTY, factories),
DataType.INTEGER, factories -> new GreatestIntEvaluator.Factory(Source.EMPTY, factories),
DataType.LONG, factories -> new GreatestLongEvaluator.Factory(Source.EMPTY, factories),
DataType.DATETIME, factories -> new GreatestLongEvaluator.Factory(Source.EMPTY, factories),
DataType.DATE_NANOS, factories -> new GreatestLongEvaluator.Factory(Source.EMPTY, factories),
DataType.IP, factories -> new GreatestBytesRefEvaluator.Factory(Source.EMPTY, factories),
DataType.VERSION, factories -> new GreatestBytesRefEvaluator.Factory(Source.EMPTY, factories)
);

@FunctionInfo(
returnType = { "boolean", "date", "date_nanos", "double", "integer", "ip", "keyword", "long", "version" },
description = "Returns the maximum value from multiple columns. This is similar to <<esql-mv_max>>\n"
Expand Down Expand Up @@ -118,6 +136,11 @@ protected TypeResolution resolveType() {
return resolution;
}
}

if (dataType != NULL && !EVALUATOR_MAP.containsKey(dataType) && !DataType.isString(dataType)) {
return new TypeResolution("Cannot use [" + dataType.typeName() + "] with function [" + getWriteableName() + "]");
}

return TypeResolution.TYPE_RESOLVED;
}

Expand All @@ -140,26 +163,24 @@ public boolean foldable() {
public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
// force datatype initialization
var dataType = dataType();
if (dataType == DataType.NULL) {
throw EsqlIllegalArgumentException.illegalDataType(dataType);
}

ExpressionEvaluator.Factory[] factories = children().stream()
.map(e -> toEvaluator.apply(new MvMax(e.source(), e)))
.toArray(ExpressionEvaluator.Factory[]::new);
if (dataType == DataType.BOOLEAN) {
return new GreatestBooleanEvaluator.Factory(source(), factories);
}
if (dataType == DataType.DOUBLE) {
return new GreatestDoubleEvaluator.Factory(source(), factories);
}
if (dataType == DataType.INTEGER) {
return new GreatestIntEvaluator.Factory(source(), factories);
}
if (dataType == DataType.LONG || dataType == DataType.DATETIME || dataType == DataType.DATE_NANOS) {
return new GreatestLongEvaluator.Factory(source(), factories);
}
if (DataType.isString(dataType) || dataType == DataType.IP || dataType == DataType.VERSION || dataType == DataType.UNSUPPORTED) {

if (DataType.isString(dataType)) {
return new GreatestBytesRefEvaluator.Factory(source(), factories);
}
throw EsqlIllegalArgumentException.illegalDataType(dataType);

var evaluatorFactory = EVALUATOR_MAP.get(dataType);
if (evaluatorFactory == null) {
throw EsqlIllegalArgumentException.illegalDataType(dataType);
}

return evaluatorFactory.apply(factories);
}

@Evaluator(extraName = "Boolean")
Expand Down
46 changes: 32 additions & 14 deletions .../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Least.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.compute.ann.Evaluator;
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Expression;
Expand All @@ -30,6 +31,10 @@

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Stream;

import static org.elasticsearch.xpack.esql.core.type.DataType.NULL;
Expand All @@ -42,6 +47,17 @@ public class Least extends EsqlScalarFunction implements OptionalArgument {

private DataType dataType;

private static final Map<DataType, Function<ExpressionEvaluator.Factory[], ExpressionEvaluator.Factory>> EVALUATOR_MAP = Map.of(
DataType.BOOLEAN, factories -> new LeastBooleanEvaluator.Factory(Source.EMPTY, factories),
DataType.DOUBLE, factories -> new LeastDoubleEvaluator.Factory(Source.EMPTY, factories),
DataType.INTEGER, factories -> new LeastIntEvaluator.Factory(Source.EMPTY, factories),
DataType.LONG, factories -> new LeastLongEvaluator.Factory(Source.EMPTY, factories),
DataType.DATETIME, factories -> new LeastLongEvaluator.Factory(Source.EMPTY, factories),
DataType.DATE_NANOS, factories -> new LeastLongEvaluator.Factory(Source.EMPTY, factories),
DataType.IP, factories -> new LeastBytesRefEvaluator.Factory(Source.EMPTY, factories),
DataType.VERSION, factories -> new LeastBytesRefEvaluator.Factory(Source.EMPTY, factories)
);

@FunctionInfo(
returnType = { "boolean", "date", "date_nanos", "double", "integer", "ip", "keyword", "long", "version" },
description = "Returns the minimum value from multiple columns. "
Expand Down Expand Up @@ -116,6 +132,11 @@ protected TypeResolution resolveType() {
return resolution;
}
}

if (dataType != NULL && !EVALUATOR_MAP.containsKey(dataType) && !DataType.isString(dataType)) {
return new TypeResolution("Cannot use [" + dataType.typeName() + "] with function [" + getWriteableName() + "]");
}

return TypeResolution.TYPE_RESOLVED;
}

Expand All @@ -138,27 +159,24 @@ public boolean foldable() {
public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
// force datatype initialization
var dataType = dataType();
if (dataType == DataType.NULL) {
throw EsqlIllegalArgumentException.illegalDataType(dataType);
}

ExpressionEvaluator.Factory[] factories = children().stream()
.map(e -> toEvaluator.apply(new MvMin(e.source(), e)))
.toArray(ExpressionEvaluator.Factory[]::new);
if (dataType == DataType.BOOLEAN) {
return new LeastBooleanEvaluator.Factory(source(), factories);
}
if (dataType == DataType.DOUBLE) {
return new LeastDoubleEvaluator.Factory(source(), factories);
}
if (dataType == DataType.INTEGER) {
return new LeastIntEvaluator.Factory(source(), factories);
}
if (dataType == DataType.LONG || dataType == DataType.DATETIME || dataType == DataType.DATE_NANOS) {
return new LeastLongEvaluator.Factory(source(), factories);
}
if (DataType.isString(dataType) || dataType == DataType.IP || dataType == DataType.VERSION || dataType == DataType.UNSUPPORTED) {

if (DataType.isString(dataType)) {
return new LeastBytesRefEvaluator.Factory(source(), factories);
}
throw EsqlIllegalArgumentException.illegalDataType(dataType);

var evaluatorFactory = EVALUATOR_MAP.get(dataType);
if (evaluatorFactory == null) {
throw EsqlIllegalArgumentException.illegalDataType(dataType);
}

return evaluatorFactory.apply(factories);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Evaluator(extraName = "Boolean")
Expand Down
Loading