From e6fd9a1944c0de3c3170e699197eec2a89f12b15 Mon Sep 17 00:00:00 2001 From: "ievgen.degtiarenko" Date: Mon, 3 Feb 2025 16:48:34 +0100 Subject: [PATCH 1/5] replace tuples with named parameters in AggregateMapper --- .../xpack/esql/planner/AggregateMapper.java | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java index a66a302354df2..c43a6619816b3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java @@ -48,6 +48,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -99,8 +100,7 @@ public AggDef withoutExtra() { /** Map of AggDef types to intermediate named expressions. */ private static final Map> MAPPER = AGG_FUNCTIONS.stream() - .flatMap(AggregateMapper::typeAndNames) - .flatMap(AggregateMapper::groupingAndNonGrouping) + .flatMap(AggregateMapper::aggDefs) .collect(Collectors.toUnmodifiableMap(aggDef -> aggDef, AggregateMapper::lookupIntermediateState)); /** Cache of aggregates to intermediate expressions. */ @@ -167,7 +167,7 @@ private static List getNonNull(AggDef aggDef) { return l; } - private static Stream, Tuple>> typeAndNames(Class clazz) { + private static Stream aggDefs(Class clazz) { List types; List extraConfigs = List.of(""); if (NumericAggregate.class.isAssignableFrom(clazz)) { @@ -197,32 +197,26 @@ private static Stream, Tuple>> typeAndNames(Class assert false : "unknown aggregate type " + clazz; throw new IllegalArgumentException("unknown aggregate type " + clazz); } - return combine(clazz, types, extraConfigs); - } - - private static Stream, Tuple>> combine(Class clazz, List types, List extraConfigs) { - return combinations(types, extraConfigs).map(combo -> new Tuple<>(clazz, combo)); + return combinations(types, extraConfigs).flatMap(typeAndExtraConfig -> { + var type = typeAndExtraConfig.v1(); + var extra = typeAndExtraConfig.v2(); + + if (clazz.isAssignableFrom(Rate.class)) { + // rate doesn't support non-grouping aggregations + return Stream.of(new AggDef(clazz, type, extra, true)); + } else if (Objects.equals(type, "AggregateMetricDouble")) { + // TODO: support grouping aggregations for aggregate metric double + return Stream.of(new AggDef(clazz, type, extra, false)); + } else { + return Stream.of(new AggDef(clazz, type, extra, true), new AggDef(clazz, type, extra, false)); + } + }); } private static Stream> combinations(List types, List extraConfigs) { return types.stream().flatMap(type -> extraConfigs.stream().map(config -> new Tuple<>(type, config))); } - private static Stream groupingAndNonGrouping(Tuple, Tuple> tuple) { - if (tuple.v1().isAssignableFrom(Rate.class)) { - // rate doesn't support non-grouping aggregations - return Stream.of(new AggDef(tuple.v1(), tuple.v2().v1(), tuple.v2().v2(), true)); - } else if (tuple.v2().v1().equals("AggregateMetricDouble")) { - // TODO: support grouping aggregations for aggregate metric double - return Stream.of(new AggDef(tuple.v1(), tuple.v2().v1(), tuple.v2().v2(), false)); - } else { - return Stream.of( - new AggDef(tuple.v1(), tuple.v2().v1(), tuple.v2().v2(), true), - new AggDef(tuple.v1(), tuple.v2().v1(), tuple.v2().v2(), false) - ); - } - } - /** Retrieves the intermediate state description for a given class, type, and grouping. */ private static List lookupIntermediateState(AggDef aggDef) { try { From 6e3d925be56ca3d1825135e520835436fae8c17f Mon Sep 17 00:00:00 2001 From: "ievgen.degtiarenko" Date: Mon, 3 Feb 2025 16:55:45 +0100 Subject: [PATCH 2/5] remove unnecessary qualifiers --- .../xpack/esql/planner/AggregateMapper.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java index c43a6619816b3..9dfe565ea90e9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java @@ -319,11 +319,11 @@ private static String dataTypeToString(DataType type, Class aggClass) { } return switch (type) { - case DataType.BOOLEAN -> "Boolean"; - case DataType.INTEGER, DataType.COUNTER_INTEGER -> "Int"; - case DataType.LONG, DataType.DATETIME, DataType.COUNTER_LONG, DataType.DATE_NANOS -> "Long"; - case DataType.DOUBLE, DataType.COUNTER_DOUBLE -> "Double"; - case DataType.KEYWORD, DataType.IP, DataType.VERSION, DataType.TEXT, DataType.SEMANTIC_TEXT -> "BytesRef"; + case BOOLEAN -> "Boolean"; + case INTEGER, COUNTER_INTEGER -> "Int"; + case LONG, DATETIME, COUNTER_LONG, DATE_NANOS -> "Long"; + case DOUBLE, COUNTER_DOUBLE -> "Double"; + case KEYWORD, IP, VERSION, TEXT, SEMANTIC_TEXT -> "BytesRef"; case GEO_POINT -> "GeoPoint"; case CARTESIAN_POINT -> "CartesianPoint"; case GEO_SHAPE -> "GeoShape"; From 075d3920938a8fb6c87021b08001f77e9cacc04a Mon Sep 17 00:00:00 2001 From: "ievgen.degtiarenko" Date: Mon, 3 Feb 2025 16:56:46 +0100 Subject: [PATCH 3/5] merge branches --- .../elasticsearch/xpack/esql/planner/AggregateMapper.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java index 9dfe565ea90e9..b2c7840aa9cea 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java @@ -311,10 +311,7 @@ private static String dataTypeToString(DataType type, Class aggClass) { if (aggClass == ToPartial.class || aggClass == FromPartial.class) { return ""; } - if ((aggClass == Max.class || aggClass == Min.class) && type.equals(DataType.IP)) { - return "Ip"; - } - if (aggClass == Top.class && type.equals(DataType.IP)) { + if ((aggClass == Max.class || aggClass == Min.class || aggClass == Top.class) && type.equals(DataType.IP)) { return "Ip"; } From aee53266d6864de1b4221591920af002cda33546 Mon Sep 17 00:00:00 2001 From: "ievgen.degtiarenko" Date: Mon, 3 Feb 2025 16:57:40 +0100 Subject: [PATCH 4/5] remove unnecessary constructor --- .../elasticsearch/xpack/esql/planner/AggregateMapper.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java index b2c7840aa9cea..87abd9caa6d0d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java @@ -104,11 +104,7 @@ public AggDef withoutExtra() { .collect(Collectors.toUnmodifiableMap(aggDef -> aggDef, AggregateMapper::lookupIntermediateState)); /** Cache of aggregates to intermediate expressions. */ - private final HashMap> cache; - - AggregateMapper() { - cache = new HashMap<>(); - } + private final HashMap> cache = new HashMap<>(); public List mapNonGrouping(List aggregates) { return doMapping(aggregates, false); From 275a1dc82c436be0483621ede71cc1e9de05b17c Mon Sep 17 00:00:00 2001 From: "ievgen.degtiarenko" Date: Mon, 3 Feb 2025 17:10:57 +0100 Subject: [PATCH 5/5] simplify package resolving --- .../xpack/esql/planner/AggregateMapper.java | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java index 87abd9caa6d0d..56ab4652bd482 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java @@ -254,23 +254,13 @@ private static MethodHandle lookupRetry(Class clazz, String type, String extr /** Determines the engines agg class name, for the given class, type, and grouping. */ private static String determineAggName(Class clazz, String type, String extra, boolean grouping) { - StringBuilder sb = new StringBuilder(); - sb.append(determinePackageName(clazz)).append("."); - sb.append(clazz.getSimpleName()); - sb.append(type); - sb.append(extra); - sb.append(grouping ? "Grouping" : ""); - sb.append("AggregatorFunction"); - return sb.toString(); - } - - /** Determines the engine agg package name, for the given class. */ - private static String determinePackageName(Class clazz) { - if (clazz.getSimpleName().startsWith("Spatial")) { - // All spatial aggs are in the spatial sub-package - return "org.elasticsearch.compute.aggregation.spatial"; - } - return "org.elasticsearch.compute.aggregation"; + return "org.elasticsearch.compute.aggregation." + + (clazz.getSimpleName().startsWith("Spatial") ? "spatial." : "") + + clazz.getSimpleName() + + type + + extra + + (grouping ? "Grouping" : "") + + "AggregatorFunction"; } /** Maps intermediate state description to named expressions. */