Skip to content

Commit 26b1ea8

Browse files
authored
[ESQL] Make AggregateMapper#mapper static (#108898)
We are currently building this hashmap whenever we create an AggregateMapper although it is always the same. Therefore let's make it static. In addition I am adjusting the visibility of the variables according to how they are accessed.
1 parent 4b50858 commit 26b1ea8

File tree

1 file changed

+14
-24
lines changed

1 file changed

+14
-24
lines changed

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,13 @@
4747
import static org.elasticsearch.xpack.esql.type.EsqlDataTypes.CARTESIAN_POINT;
4848
import static org.elasticsearch.xpack.esql.type.EsqlDataTypes.GEO_POINT;
4949

50-
public class AggregateMapper {
50+
final class AggregateMapper {
5151

52-
static final List<String> NUMERIC = List.of("Int", "Long", "Double");
53-
static final List<String> SPATIAL = List.of("GeoPoint", "CartesianPoint");
52+
private static final List<String> NUMERIC = List.of("Int", "Long", "Double");
53+
private static final List<String> SPATIAL = List.of("GeoPoint", "CartesianPoint");
5454

5555
/** List of all mappable ESQL agg functions (excludes surrogates like AVG = SUM/COUNT). */
56-
static final List<? extends Class<? extends Function>> AGG_FUNCTIONS = List.of(
56+
private static final List<? extends Class<? extends Function>> AGG_FUNCTIONS = List.of(
5757
Count.class,
5858
CountDistinct.class,
5959
Max.class,
@@ -66,23 +66,19 @@ public class AggregateMapper {
6666
);
6767

6868
/** Record of agg Class, type, and grouping (or non-grouping). */
69-
record AggDef(Class<?> aggClazz, String type, String extra, boolean grouping) {}
69+
private record AggDef(Class<?> aggClazz, String type, String extra, boolean grouping) {}
7070

7171
/** Map of AggDef types to intermediate named expressions. */
72-
private final Map<AggDef, List<IntermediateStateDesc>> mapper;
72+
private static final Map<AggDef, List<IntermediateStateDesc>> mapper = AGG_FUNCTIONS.stream()
73+
.flatMap(AggregateMapper::typeAndNames)
74+
.flatMap(AggregateMapper::groupingAndNonGrouping)
75+
.collect(Collectors.toUnmodifiableMap(aggDef -> aggDef, AggregateMapper::lookupIntermediateState));
7376

7477
/** Cache of aggregates to intermediate expressions. */
75-
private final HashMap<Expression, List<? extends NamedExpression>> cache = new HashMap<>();
78+
private final HashMap<Expression, List<? extends NamedExpression>> cache;
7679

7780
AggregateMapper() {
78-
this(AGG_FUNCTIONS);
79-
}
80-
81-
AggregateMapper(List<? extends Class<? extends Function>> aggregateFunctionClasses) {
82-
mapper = aggregateFunctionClasses.stream()
83-
.flatMap(AggregateMapper::typeAndNames)
84-
.flatMap(AggregateMapper::groupingAndNonGrouping)
85-
.collect(Collectors.toUnmodifiableMap(aggDef -> aggDef, AggregateMapper::lookupIntermediateState));
81+
cache = new HashMap<>();
8682
}
8783

8884
public List<? extends NamedExpression> mapNonGrouping(List<? extends Expression> aggregates) {
@@ -108,11 +104,10 @@ public List<? extends NamedExpression> mapGrouping(Expression aggregate) {
108104
}
109105

110106
private Stream<? extends NamedExpression> map(Expression aggregate, boolean grouping) {
111-
aggregate = Alias.unwrap(aggregate);
112-
return cache.computeIfAbsent(aggregate, aggKey -> computeEntryForAgg(aggKey, grouping)).stream();
107+
return cache.computeIfAbsent(Alias.unwrap(aggregate), aggKey -> computeEntryForAgg(aggKey, grouping)).stream();
113108
}
114109

115-
private List<? extends NamedExpression> computeEntryForAgg(Expression aggregate, boolean grouping) {
110+
private static List<? extends NamedExpression> computeEntryForAgg(Expression aggregate, boolean grouping) {
116111
var aggDef = aggDefOrNull(aggregate, grouping);
117112
if (aggDef != null) {
118113
var is = getNonNull(aggDef);
@@ -128,7 +123,7 @@ private List<? extends NamedExpression> computeEntryForAgg(Expression aggregate,
128123
}
129124

130125
/** Gets the agg from the mapper - wrapper around map::get for more informative failure.*/
131-
private List<IntermediateStateDesc> getNonNull(AggDef aggDef) {
126+
private static List<IntermediateStateDesc> getNonNull(AggDef aggDef) {
132127
var l = mapper.get(aggDef);
133128
if (l == null) {
134129
throw new EsqlIllegalArgumentException("Cannot find intermediate state for: " + aggDef);
@@ -268,9 +263,4 @@ private static String dataTypeToString(DataType type, Class<?> aggClass) {
268263
throw new EsqlIllegalArgumentException("illegal agg type: " + type.typeName());
269264
}
270265
}
271-
272-
private static Expression unwrapAlias(Expression expression) {
273-
if (expression instanceof Alias alias) return alias.child();
274-
return expression;
275-
}
276266
}

0 commit comments

Comments
 (0)