From 37a37165e45f4c9991581c65d00f3410b99a210e Mon Sep 17 00:00:00 2001 From: Dennis Oelkers Date: Wed, 13 Nov 2024 09:45:16 +0100 Subject: [PATCH] Generating proper aggregation sort for if multiple pivots. --- .../pivot/ESPivotBucketSpecHandler.java | 38 ++++++++++++++++--- .../pivot/OSPivotBucketSpecHandler.java | 38 ++++++++++++++++--- .../pivot/buckets/OSTimeHandler.java | 7 ++-- .../pivot/buckets/OSValuesHandler.java | 35 ++--------------- 4 files changed, 72 insertions(+), 46 deletions(-) diff --git a/graylog-storage-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/searchtypes/pivot/ESPivotBucketSpecHandler.java b/graylog-storage-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/searchtypes/pivot/ESPivotBucketSpecHandler.java index 3d0d385f64f4..46ef348d47ea 100644 --- a/graylog-storage-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/searchtypes/pivot/ESPivotBucketSpecHandler.java +++ b/graylog-storage-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/searchtypes/pivot/ESPivotBucketSpecHandler.java @@ -26,9 +26,11 @@ import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec; import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.Aggregation; import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.AggregationBuilder; +import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.AggregationBuilders; import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.BucketOrder; import org.graylog.storage.elasticsearch7.views.ESGeneratedQueryContext; +import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -46,12 +48,24 @@ protected void record(ESGeneratedQueryContext queryContext, Pivot pivot, PivotSp aggTypes(queryContext, pivot).record(spec, name, aggregationClass); } - protected List orderListForPivot(Pivot pivot, ESGeneratedQueryContext esGeneratedQueryContext, BucketOrder defaultOrder) { + public record SortOrders(List orders, List subAggregations) {} + + protected SortOrders orderListForPivot(Pivot pivot, ESGeneratedQueryContext esGeneratedQueryContext, BucketOrder defaultOrder) { + final List subaggregations = new ArrayList<>(); final List ordering = pivot.sort() .stream() .map(sortSpec -> { - if (sortSpec instanceof PivotSort) { - return BucketOrder.key(sortSpec.direction().equals(SortSpec.Direction.Ascending)); + final var isAscending = sortSpec.direction().equals(SortSpec.Direction.Ascending); + if (sortSpec instanceof PivotSort pivotSort) { + if (isSortOnNumericPivotField(pivot, pivotSort, esGeneratedQueryContext)) { + /* When we sort on a numeric pivot field, we create a metric sub-aggregation for that field, which returns + the numeric value of it, so that we can sort on it numerically. Any metric aggregation (min/max/avg) will work. */ + final var aggregationName = "sort_helper" + pivotSort.field(); + subaggregations.add(AggregationBuilders.max(aggregationName).field(pivotSort.field())); + return BucketOrder.aggregation(aggregationName, isAscending); + } else { + return BucketOrder.key(isAscending); + } } if (sortSpec instanceof SeriesSort) { final Optional matchingSeriesSpec = pivot.series() @@ -61,14 +75,14 @@ protected List orderListForPivot(Pivot pivot, ESGeneratedQueryConte return matchingSeriesSpec .map(seriesSpec -> { if (seriesSpec.literal().equals("count()")) { - return BucketOrder.count(sortSpec.direction().equals(SortSpec.Direction.Ascending)); + return BucketOrder.count(isAscending); } String orderPath = seriesSpec.statsSubfieldName() .map(subField -> esGeneratedQueryContext.seriesName(seriesSpec, pivot) + "." + subField) .orElse(esGeneratedQueryContext.seriesName(seriesSpec, pivot)); - return BucketOrder.aggregation(orderPath, sortSpec.direction().equals(SortSpec.Direction.Ascending)); + return BucketOrder.aggregation(orderPath, isAscending); }) .orElse(null); } @@ -77,7 +91,19 @@ protected List orderListForPivot(Pivot pivot, ESGeneratedQueryConte }) .filter(Objects::nonNull) .collect(Collectors.toList()); - return ordering.isEmpty() ? List.of(defaultOrder) : ordering; + return ordering.isEmpty() + ? new SortOrders(List.of(defaultOrder), List.of()) + : new SortOrders(ordering, List.copyOf(subaggregations)); + } + + private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, ESGeneratedQueryContext queryContext) { + return queryContext.fieldType(pivot.effectiveStreams(), pivotSort.field()) + .filter(this::isNumericFieldType) + .isPresent(); + } + + private boolean isNumericFieldType(String fieldType) { + return fieldType.equals("long") || fieldType.equals("double") || fieldType.equals("float"); } public abstract Stream extractBuckets(Pivot pivot, BucketSpec bucketSpec, PivotBucket initialBucket); diff --git a/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/OSPivotBucketSpecHandler.java b/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/OSPivotBucketSpecHandler.java index 5c6e09ed1d2b..51ebc3c6b793 100644 --- a/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/OSPivotBucketSpecHandler.java +++ b/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/OSPivotBucketSpecHandler.java @@ -26,9 +26,11 @@ import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec; import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.Aggregation; import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilder; +import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilders; import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.BucketOrder; import org.graylog.storage.opensearch2.views.OSGeneratedQueryContext; +import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -46,12 +48,24 @@ protected void record(OSGeneratedQueryContext queryContext, Pivot pivot, PivotSp aggTypes(queryContext, pivot).record(spec, name, aggregationClass); } - protected List orderListForPivot(Pivot pivot, OSGeneratedQueryContext queryContext, BucketOrder defaultOrder) { + public record SortOrders(List orders, List subAggregations) {} + + protected SortOrders orderListForPivot(Pivot pivot, OSGeneratedQueryContext queryContext, BucketOrder defaultOrder) { + final List subaggregations = new ArrayList<>(); final List ordering = pivot.sort() .stream() .map(sortSpec -> { - if (sortSpec instanceof PivotSort) { - return BucketOrder.key(sortSpec.direction().equals(SortSpec.Direction.Ascending)); + final var isAscending = sortSpec.direction().equals(SortSpec.Direction.Ascending); + if (sortSpec instanceof PivotSort pivotSort) { + if (isSortOnNumericPivotField(pivot, pivotSort, queryContext)) { + /* When we sort on a numeric pivot field, we create a metric sub-aggregation for that field, which returns + the numeric value of it, so that we can sort on it numerically. Any metric aggregation (min/max/avg) will work. */ + final var aggregationName = "sort_helper" + pivotSort.field(); + subaggregations.add(AggregationBuilders.max(aggregationName).field(pivotSort.field())); + return BucketOrder.aggregation(aggregationName, isAscending); + } else { + return BucketOrder.key(isAscending); + } } if (sortSpec instanceof SeriesSort) { final Optional matchingSeriesSpec = pivot.series() @@ -61,13 +75,13 @@ protected List orderListForPivot(Pivot pivot, OSGeneratedQueryConte return matchingSeriesSpec .map(seriesSpec -> { if (seriesSpec.literal().equals("count()")) { - return BucketOrder.count(sortSpec.direction().equals(SortSpec.Direction.Ascending)); + return BucketOrder.count(isAscending); } String orderPath = seriesSpec.statsSubfieldName() .map(subField -> queryContext.seriesName(seriesSpec, pivot) + "." + subField) .orElse(queryContext.seriesName(seriesSpec, pivot)); - return BucketOrder.aggregation(orderPath, sortSpec.direction().equals(SortSpec.Direction.Ascending)); + return BucketOrder.aggregation(orderPath, isAscending); }) .orElse(null); } @@ -76,7 +90,19 @@ protected List orderListForPivot(Pivot pivot, OSGeneratedQueryConte }) .filter(Objects::nonNull) .collect(Collectors.toList()); - return ordering.isEmpty() ? List.of(defaultOrder) : ordering; + return ordering.isEmpty() + ? new SortOrders(List.of(defaultOrder), List.of()) + : new SortOrders(ordering, List.copyOf(subaggregations)); + } + + private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, OSGeneratedQueryContext queryContext) { + return queryContext.fieldType(pivot.effectiveStreams(), pivotSort.field()) + .filter(this::isNumericFieldType) + .isPresent(); + } + + private boolean isNumericFieldType(String fieldType) { + return fieldType.equals("long") || fieldType.equals("double") || fieldType.equals("float"); } public abstract Stream extractBuckets(Pivot pivot, BucketSpec bucketSpecs, PivotBucket previousBucket); diff --git a/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/buckets/OSTimeHandler.java b/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/buckets/OSTimeHandler.java index 423e59490cf9..0bbc9aa83769 100644 --- a/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/buckets/OSTimeHandler.java +++ b/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/buckets/OSTimeHandler.java @@ -37,7 +37,6 @@ import org.graylog2.plugin.indexer.searches.timeranges.TimeRange; import javax.annotation.Nonnull; -import java.util.List; import java.util.stream.Stream; public class OSTimeHandler extends OSPivotBucketSpecHandler