From 6cb6ac968f99d92486767d069a41794a0f829edd Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 8 Jul 2025 15:13:07 -0700 Subject: [PATCH] Fix empty VALUES with ordinals grouping (#130861) We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase. Relates #130576 (cherry picked from commit f58d2916e283e8350627f412eef1619b48515b32) --- .../compute/aggregation/ValuesBytesRefAggregator.java | 2 +- .../compute/aggregation/ValuesDoubleAggregator.java | 2 +- .../compute/aggregation/ValuesFloatAggregator.java | 2 +- .../elasticsearch/compute/aggregation/ValuesIntAggregator.java | 2 +- .../elasticsearch/compute/aggregation/ValuesLongAggregator.java | 2 +- .../compute/aggregation/X-ValuesAggregator.java.st | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java index 72c840f090b83..de178ce8a971e 100644 --- a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java +++ b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregator.java @@ -71,10 +71,10 @@ public static void combineIntermediate(GroupingState state, int groupId, BytesRe } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) { diff --git a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java index 99919b3b05158..786b5c0857548 100644 --- a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java +++ b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesDoubleAggregator.java @@ -67,10 +67,10 @@ public static void combineIntermediate(GroupingState state, int groupId, DoubleB } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) { diff --git a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java index e544635de2ff5..7ed479a4d69ca 100644 --- a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java +++ b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesFloatAggregator.java @@ -66,10 +66,10 @@ public static void combineIntermediate(GroupingState state, int groupId, FloatBl } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) { diff --git a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java index 0ef8923a812ff..dd3af8af09a05 100644 --- a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java +++ b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesIntAggregator.java @@ -66,10 +66,10 @@ public static void combineIntermediate(GroupingState state, int groupId, IntBloc } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) { diff --git a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java index 7a0a8bed86fe9..97bbfc3f8365f 100644 --- a/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java +++ b/x-pack/plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/aggregation/ValuesLongAggregator.java @@ -67,10 +67,10 @@ public static void combineIntermediate(GroupingState state, int groupId, LongBlo } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) { diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st index ecf8d8180e168..89f30d8811107 100644 --- a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st @@ -104,10 +104,10 @@ $endif$ } public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { - var sorted = state.sortedForOrdinalMerging(current); if (statePosition > state.maxGroupId) { return; } + var sorted = state.sortedForOrdinalMerging(current); var start = statePosition > 0 ? sorted.counts[statePosition - 1] : 0; var end = sorted.counts[statePosition]; for (int i = start; i < end; i++) {