Skip to content

Commit f90f857

Browse files
luk-kaminskiluk-kaminski
authored
Fixed sorting on percentile metric (#24524)
* Fixed sorting on percentile metric * Renaming method to be more precise. Applying the fix in other storage modules. * Changelog added * Found a way sorting can work on percentile with real numbers (although FE does not use it, may be important for Scripting API) * Minor refactoring and TODO explaining potential improvements in the future
1 parent b084385 commit f90f857

File tree

9 files changed

+62
-31
lines changed

9 files changed

+62
-31
lines changed

‎changelog/unreleased/pr-24524.toml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
type = "fixed"
2+
message = "Fixes back-end problems with sorting by percentile in pivots. Before this change, default sorting was used even if sorting by percentile was requested."
3+
4+
issues = ["8863"]
5+
pulls = ["24524"]
6+
7+

‎graylog-storage-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/searchtypes/pivot/ESPivotBucketSpecHandler.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
package org.graylog.storage.elasticsearch7.views.searchtypes.pivot;
1818

1919
import org.graylog.plugins.views.search.Query;
20+
import org.graylog.plugins.views.search.engine.IndexerGeneratedQueryContext;
2021
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpec;
2122
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpecHandler;
2223
import org.graylog.plugins.views.search.searchtypes.pivot.Pivot;
2324
import org.graylog.plugins.views.search.searchtypes.pivot.PivotSort;
2425
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSort;
2526
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSpec;
2627
import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec;
28+
import org.graylog.plugins.views.search.searchtypes.pivot.series.Percentile;
2729
import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.AggregationBuilder;
2830
import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.AggregationBuilders;
2931
import org.graylog.shaded.elasticsearch7.org.elasticsearch.search.aggregations.BucketOrder;
@@ -41,14 +43,14 @@ public abstract class ESPivotBucketSpecHandler<SPEC_TYPE extends BucketSpec>
4143

4244
public record SortOrders(List<BucketOrder> orders, List<AggregationBuilder> sortingAggregations) {}
4345

44-
protected SortOrders orderListForPivot(Pivot pivot, ESGeneratedQueryContext esGeneratedQueryContext, BucketOrder defaultOrder, Query query) {
46+
protected SortOrders orderListForPivot(Pivot pivot, IndexerGeneratedQueryContext<?> queryContext, BucketOrder defaultOrder, Query query) {
4547
final List<AggregationBuilder> sortingAggregations = new ArrayList<>();
4648
final List<BucketOrder> ordering = pivot.sort()
4749
.stream()
4850
.map(sortSpec -> {
4951
final var isAscending = sortSpec.direction().equals(SortSpec.Direction.Ascending);
5052
if (sortSpec instanceof PivotSort pivotSort) {
51-
if (isSortOnNumericPivotField(pivot, pivotSort, esGeneratedQueryContext, query)) {
53+
if (isSortOnNumericPivotField(pivot, pivotSort, queryContext, query)) {
5254
/* When we sort on a numeric pivot field, we create a metric sub-aggregation for that field, which returns
5355
the numeric value of it, so that we can sort on it numerically. Any metric aggregation (min/max/avg) will work. */
5456
final var aggregationName = "sort_helper" + pivotSort.field();
@@ -61,19 +63,23 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
6163
if (sortSpec instanceof SeriesSort) {
6264
final Optional<SeriesSpec> matchingSeriesSpec = pivot.series()
6365
.stream()
64-
.filter(series -> series.literal().equals(sortSpec.field()))
66+
.filter(series ->
67+
series.literal().equals(sortSpec.field())
68+
|| (series instanceof Percentile && sortSpec.field().equals(series.id())) //TODO: possibly could be removed if FE used real numbers instead of integers in percentile series sort field
69+
)
6570
.findFirst();
6671
return matchingSeriesSpec
6772
.map(seriesSpec -> {
6873
if (seriesSpec.literal().equals("count()")) {
6974
return BucketOrder.count(isAscending);
70-
}
75+
} else {
7176

72-
String orderPath = seriesSpec.statsSubfieldName()
73-
.map(subField -> esGeneratedQueryContext.seriesName(seriesSpec, pivot) + "." + subField)
74-
.orElse(esGeneratedQueryContext.seriesName(seriesSpec, pivot));
77+
String orderPath = seriesSpec.multiValueAggSubfieldName()
78+
.map(subField -> queryContext.seriesName(seriesSpec, pivot) + "[" + subField + "]")
79+
.orElse(queryContext.seriesName(seriesSpec, pivot));
7580

76-
return BucketOrder.aggregation(orderPath, isAscending);
81+
return BucketOrder.aggregation(orderPath, isAscending);
82+
}
7783
})
7884
.orElse(null);
7985
}
@@ -87,7 +93,7 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
8793
: new SortOrders(ordering, List.copyOf(sortingAggregations));
8894
}
8995

90-
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, ESGeneratedQueryContext queryContext, Query query) {
96+
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, IndexerGeneratedQueryContext<?> queryContext, Query query) {
9197
return queryContext.fieldType(query.effectiveStreams(pivot), pivotSort.field())
9298
.filter(this::isNumericFieldType)
9399
.isPresent();

‎graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/views/searchtypes/pivot/OSPivotBucketSpecHandler.java‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
package org.graylog.storage.opensearch2.views.searchtypes.pivot;
1818

1919
import org.graylog.plugins.views.search.Query;
20+
import org.graylog.plugins.views.search.engine.IndexerGeneratedQueryContext;
2021
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpec;
2122
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpecHandler;
2223
import org.graylog.plugins.views.search.searchtypes.pivot.Pivot;
2324
import org.graylog.plugins.views.search.searchtypes.pivot.PivotSort;
2425
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSort;
2526
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSpec;
2627
import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec;
28+
import org.graylog.plugins.views.search.searchtypes.pivot.series.Percentile;
2729
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilder;
2830
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilders;
2931
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.BucketOrder;
@@ -41,7 +43,7 @@ public abstract class OSPivotBucketSpecHandler<SPEC_TYPE extends BucketSpec>
4143

4244
public record SortOrders(List<BucketOrder> orders, List<AggregationBuilder> sortingAggregations) {}
4345

44-
protected SortOrders orderListForPivot(Pivot pivot, OSGeneratedQueryContext queryContext, BucketOrder defaultOrder, Query query) {
46+
protected SortOrders orderListForPivot(Pivot pivot, IndexerGeneratedQueryContext<?> queryContext, BucketOrder defaultOrder, Query query) {
4547
final List<AggregationBuilder> sortingAggregations = new ArrayList<>();
4648
final List<BucketOrder> ordering = pivot.sort()
4749
.stream()
@@ -61,18 +63,22 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
6163
if (sortSpec instanceof SeriesSort) {
6264
final Optional<SeriesSpec> matchingSeriesSpec = pivot.series()
6365
.stream()
64-
.filter(series -> series.literal().equals(sortSpec.field()))
66+
.filter(series ->
67+
series.literal().equals(sortSpec.field())
68+
|| (series instanceof Percentile && sortSpec.field().equals(series.id())) //TODO: possibly could be removed if FE used real numbers instead of integers in percentile series sort field
69+
)
6570
.findFirst();
6671
return matchingSeriesSpec
6772
.map(seriesSpec -> {
6873
if (seriesSpec.literal().equals("count()")) {
6974
return BucketOrder.count(isAscending);
70-
}
71-
String orderPath = seriesSpec.statsSubfieldName()
72-
.map(subField -> queryContext.seriesName(seriesSpec, pivot) + "." + subField)
73-
.orElse(queryContext.seriesName(seriesSpec, pivot));
75+
} else {
76+
String orderPath = seriesSpec.multiValueAggSubfieldName()
77+
.map(subField -> queryContext.seriesName(seriesSpec, pivot) + "[" + subField + "]")
78+
.orElse(queryContext.seriesName(seriesSpec, pivot));
7479

75-
return BucketOrder.aggregation(orderPath, isAscending);
80+
return BucketOrder.aggregation(orderPath, isAscending);
81+
}
7682
})
7783
.orElse(null);
7884
}
@@ -86,7 +92,7 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
8692
: new SortOrders(ordering, List.copyOf(sortingAggregations));
8793
}
8894

89-
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, OSGeneratedQueryContext queryContext, Query query) {
95+
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, IndexerGeneratedQueryContext<?> queryContext, Query query) {
9096
return queryContext.fieldType(query.effectiveStreams(pivot), pivotSort.field())
9197
.filter(this::isNumericFieldType)
9298
.isPresent();

‎graylog-storage-opensearch3/src/main/java/org/graylog/storage/opensearch3/views/searchtypes/pivot/OSPivotBucketSpecHandler.java‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
package org.graylog.storage.opensearch3.views.searchtypes.pivot;
1818

1919
import org.graylog.plugins.views.search.Query;
20+
import org.graylog.plugins.views.search.engine.IndexerGeneratedQueryContext;
2021
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpec;
2122
import org.graylog.plugins.views.search.searchtypes.pivot.BucketSpecHandler;
2223
import org.graylog.plugins.views.search.searchtypes.pivot.Pivot;
2324
import org.graylog.plugins.views.search.searchtypes.pivot.PivotSort;
2425
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSort;
2526
import org.graylog.plugins.views.search.searchtypes.pivot.SeriesSpec;
2627
import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec;
28+
import org.graylog.plugins.views.search.searchtypes.pivot.series.Percentile;
2729
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilder;
2830
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.AggregationBuilders;
2931
import org.graylog.shaded.opensearch2.org.opensearch.search.aggregations.BucketOrder;
@@ -41,7 +43,7 @@ public abstract class OSPivotBucketSpecHandler<SPEC_TYPE extends BucketSpec>
4143

4244
public record SortOrders(List<BucketOrder> orders, List<AggregationBuilder> sortingAggregations) {}
4345

44-
protected SortOrders orderListForPivot(Pivot pivot, OSGeneratedQueryContext queryContext, BucketOrder defaultOrder, Query query) {
46+
protected SortOrders orderListForPivot(Pivot pivot, IndexerGeneratedQueryContext<?> queryContext, BucketOrder defaultOrder, Query query) {
4547
final List<AggregationBuilder> sortingAggregations = new ArrayList<>();
4648
final List<BucketOrder> ordering = pivot.sort()
4749
.stream()
@@ -61,18 +63,22 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
6163
if (sortSpec instanceof SeriesSort) {
6264
final Optional<SeriesSpec> matchingSeriesSpec = pivot.series()
6365
.stream()
64-
.filter(series -> series.literal().equals(sortSpec.field()))
66+
.filter(series ->
67+
series.literal().equals(sortSpec.field())
68+
|| (series instanceof Percentile && sortSpec.field().equals(series.id())) //TODO: possibly could be removed if FE used real numbers instead of integers in percentile series sort field
69+
)
6570
.findFirst();
6671
return matchingSeriesSpec
6772
.map(seriesSpec -> {
6873
if (seriesSpec.literal().equals("count()")) {
6974
return BucketOrder.count(isAscending);
70-
}
71-
String orderPath = seriesSpec.statsSubfieldName()
72-
.map(subField -> queryContext.seriesName(seriesSpec, pivot) + "." + subField)
73-
.orElse(queryContext.seriesName(seriesSpec, pivot));
75+
} else {
76+
String orderPath = seriesSpec.multiValueAggSubfieldName()
77+
.map(subField -> queryContext.seriesName(seriesSpec, pivot) + "[" + subField + "]")
78+
.orElse(queryContext.seriesName(seriesSpec, pivot));
7479

75-
return BucketOrder.aggregation(orderPath, isAscending);
80+
return BucketOrder.aggregation(orderPath, isAscending);
81+
}
7682
})
7783
.orElse(null);
7884
}
@@ -86,7 +92,7 @@ the numeric value of it, so that we can sort on it numerically. Any metric aggre
8692
: new SortOrders(ordering, List.copyOf(sortingAggregations));
8793
}
8894

89-
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, OSGeneratedQueryContext queryContext, Query query) {
95+
private boolean isSortOnNumericPivotField(Pivot pivot, PivotSort pivotSort, IndexerGeneratedQueryContext<?> queryContext, Query query) {
9096
return queryContext.fieldType(query.effectiveStreams(pivot), pivotSort.field())
9197
.filter(this::isNumericFieldType)
9298
.isPresent();

‎graylog2-server/src/main/java/org/graylog/plugins/views/search/searchtypes/pivot/SeriesSpec.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,11 @@ default String literal() {
5151
SeriesSpec withId(String id);
5252

5353
/**
54-
* Override if your series are based on "stats" or "extended stats" aggregation,
55-
* to give exact name of the subfield of those aggregations which represents the series.
54+
* Override if your series are based on multi-value metrics aggregation (i.e. "stats", "extended stats", "percentile"),
55+
* to give the exact name of the subfield of those aggregations which represents the series.
56+
* For other series - just use the default implementation, returning empty optional.
5657
*/
57-
default Optional<String> statsSubfieldName() {
58+
default Optional<String> multiValueAggSubfieldName() {
5859
return Optional.empty();
5960
}
6061

‎graylog2-server/src/main/java/org/graylog/plugins/views/search/searchtypes/pivot/series/Percentile.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ public String literal() {
4949
return type() + "(" + field() + "," + percentile() + ")";
5050
}
5151

52+
@Override
53+
public Optional<String> multiValueAggSubfieldName() {
54+
return Optional.of(String.valueOf(percentile()));
55+
}
56+
5257
public abstract Builder toBuilder();
5358

5459
@Override

‎graylog2-server/src/main/java/org/graylog/plugins/views/search/searchtypes/pivot/series/StdDev.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public abstract class StdDev implements SeriesSpec, HasField {
4343
public abstract String field();
4444

4545
@Override
46-
public Optional<String> statsSubfieldName() {
46+
public Optional<String> multiValueAggSubfieldName() {
4747
return Optional.of("std_deviation");
4848
}
4949

‎graylog2-server/src/main/java/org/graylog/plugins/views/search/searchtypes/pivot/series/SumOfSquares.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public abstract class SumOfSquares implements SeriesSpec, HasField {
4343
public abstract String field();
4444

4545
@Override
46-
public Optional<String> statsSubfieldName() {
46+
public Optional<String> multiValueAggSubfieldName() {
4747
return Optional.of("sum_of_squares");
4848
}
4949

‎graylog2-server/src/main/java/org/graylog/plugins/views/search/searchtypes/pivot/series/Variance.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public String literal() {
4848
}
4949

5050
@Override
51-
public Optional<String> statsSubfieldName() {
51+
public Optional<String> multiValueAggSubfieldName() {
5252
return Optional.of(NAME);
5353
}
5454

0 commit comments

Comments
 (0)