Skip to content

Commit b3afbe7

Browse files
committed
Revert "Continue realizing sorting by aggregations (#52298)"
This reverts commit 5659e72. It causes some test failures that didn't come up in the PR tests.
1 parent 5659e72 commit b3afbe7

File tree

8 files changed

+92
-130
lines changed

8 files changed

+92
-130
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/Aggregator.java

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,9 @@
2828
import org.elasticsearch.common.xcontent.DeprecationHandler;
2929
import org.elasticsearch.common.xcontent.XContentParser;
3030
import org.elasticsearch.search.aggregations.bucket.BucketsAggregator;
31-
import org.elasticsearch.search.aggregations.support.AggregationPath;
3231
import org.elasticsearch.search.internal.SearchContext;
3332

3433
import java.io.IOException;
35-
import java.util.Iterator;
3634

3735
/**
3836
* An Aggregator.
@@ -93,53 +91,6 @@ public static boolean descendsFromBucketAggregator(Aggregator parent) {
9391
*/
9492
public abstract Aggregator subAggregator(String name);
9593

96-
/**
97-
* Resolve the next step of the sort path as though this aggregation
98-
* supported sorting. This is usually the "first step" when resolving
99-
* a sort path because most aggs that support sorting their buckets
100-
* aren't valid in the middle of a sort path.
101-
* <p>
102-
* For example, the {@code terms} aggs supports sorting its buckets, but
103-
* that sort path itself can't contain a different {@code terms}
104-
* aggregation.
105-
*/
106-
public final Aggregator resolveSortPathOnValidAgg(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) {
107-
Aggregator n = subAggregator(next.name);
108-
if (n == null) {
109-
throw new IllegalArgumentException("The provided aggregation [" + next + "] either does not exist, or is "
110-
+ "a pipeline aggregation and cannot be used to sort the buckets.");
111-
}
112-
if (false == path.hasNext()) {
113-
return n;
114-
}
115-
if (next.key != null) {
116-
throw new IllegalArgumentException("Key only allowed on last aggregation path element but got [" + next + "]");
117-
}
118-
return n.resolveSortPath(path.next(), path);
119-
}
120-
121-
/**
122-
* Resolve a sort path to the target.
123-
* <p>
124-
* The default implementation throws an exception but we override it on aggregations that support sorting.
125-
*/
126-
public Aggregator resolveSortPath(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) {
127-
throw new IllegalArgumentException("Buckets can only be sorted on a sub-aggregator path " +
128-
"that is built out of zero or more single-bucket aggregations within the path and a final " +
129-
"single-bucket or a metrics aggregation at the path end. [" + name() + "] is not single-bucket.");
130-
}
131-
132-
/**
133-
* Validates the "key" portion of a sort on this aggregation.
134-
* <p>
135-
* The default implementation throws an exception but we override it on aggregations that support sorting.
136-
*/
137-
public void validateSortPathKey(String key) {
138-
throw new IllegalArgumentException("Buckets can only be sorted on a sub-aggregator path " +
139-
"that is built out of zero or more single-bucket aggregations within the path and a final " +
140-
"single-bucket or a metrics aggregation at the path end.");
141-
}
142-
14394
/**
14495
* Build an aggregation for data that has been collected into {@code bucket}.
14596
*/

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,10 @@
2828
import org.elasticsearch.search.aggregations.InternalAggregations;
2929
import org.elasticsearch.search.aggregations.LeafBucketCollector;
3030
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
31-
import org.elasticsearch.search.aggregations.support.AggregationPath;
3231
import org.elasticsearch.search.internal.SearchContext;
3332

3433
import java.io.IOException;
3534
import java.util.Arrays;
36-
import java.util.Iterator;
3735
import java.util.List;
3836
import java.util.Map;
3937
import java.util.function.IntConsumer;
@@ -165,24 +163,4 @@ public final void close() {
165163
}
166164
}
167165

168-
@Override
169-
public Aggregator resolveSortPath(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) {
170-
if (this instanceof SingleBucketAggregator) {
171-
return resolveSortPathOnValidAgg(next, path);
172-
}
173-
return super.resolveSortPath(next, path);
174-
}
175-
176-
@Override
177-
public void validateSortPathKey(String key) {
178-
if (false == this instanceof SingleBucketAggregator) {
179-
super.validateSortPathKey(key);
180-
return;
181-
}
182-
if (key != null && false == "doc_count".equals(key)) {
183-
throw new IllegalArgumentException("Ordering on a single-bucket aggregation can only be done on its doc_count. " +
184-
"Either drop the key (a la \"" + name() + "\") or change it to \"doc_count\" (a la \"" + name() +
185-
".doc_count\")");
186-
}
187-
}
188166
}

server/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,9 @@
2525
import org.elasticsearch.search.aggregations.BucketCollector;
2626
import org.elasticsearch.search.aggregations.InternalAggregation;
2727
import org.elasticsearch.search.aggregations.LeafBucketCollector;
28-
import org.elasticsearch.search.aggregations.support.AggregationPath.PathElement;
2928
import org.elasticsearch.search.internal.SearchContext;
3029

3130
import java.io.IOException;
32-
import java.util.Iterator;
3331

3432
/**
3533
* A {@link BucketCollector} that records collected doc IDs and buckets and
@@ -122,15 +120,6 @@ public void postCollection() throws IOException {
122120
"Deferred collectors cannot be collected directly. They must be collected through the recording wrapper.");
123121
}
124122

125-
@Override
126-
public Aggregator resolveSortPath(PathElement next, Iterator<PathElement> path) {
127-
return in.resolveSortPath(next, path);
128-
}
129-
130-
@Override
131-
public void validateSortPathKey(String key) {
132-
in.validateSortPathKey(key);
133-
}
134123
}
135124

136125
}

server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ public Object getProperty(List<String> path) {
107107
@Override
108108
public final double sortValue(String key) {
109109
if (key == null) {
110-
throw new IllegalArgumentException("Missing value key in [" + key + "] which refers to a multi-value metric aggregation");
110+
throw new IllegalArgumentException("Missing value key in [" + key+ "] which refers to a multi-value metric aggregation");
111111
}
112112
return value(key);
113113
}

server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregator.java

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,6 @@ protected SingleValue(String name, SearchContext context, Aggregator parent, Lis
4141
}
4242

4343
public abstract double metric(long owningBucketOrd);
44-
45-
@Override
46-
public void validateSortPathKey(String key) {
47-
if (key != null && false == "value".equals(key)) {
48-
throw new IllegalArgumentException("Ordering on a single-value metrics aggregation can only be done on its value. " +
49-
"Either drop the key (a la \"" + name() + "\") or change it to \"value\" (a la \"" + name() + ".value\")");
50-
}
51-
}
5244
}
5345

5446
public abstract static class MultiValue extends NumericMetricsAggregator {
@@ -61,16 +53,5 @@ protected MultiValue(String name, SearchContext context, Aggregator parent, List
6153
public abstract boolean hasMetric(String name);
6254

6355
public abstract double metric(String name, long owningBucketOrd);
64-
65-
@Override
66-
public void validateSortPathKey(String key) {
67-
if (key == null) {
68-
throw new IllegalArgumentException("When ordering on a multi-value metrics aggregation a metric name must be specified.");
69-
}
70-
if (false == hasMetric(key)) {
71-
throw new IllegalArgumentException(
72-
"Unknown metric name [" + key + "] on multi-value metrics aggregation [" + name() + "]");
73-
}
74-
}
7556
}
7657
}

server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationPath.java

Lines changed: 90 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public List<String> getPathElementsAsStringList() {
182182
return stringPathElements;
183183
}
184184

185-
private AggregationPath subPath(int offset, int length) {
185+
public AggregationPath subPath(int offset, int length) {
186186
List<PathElement> subTokens = new ArrayList<>(pathElements.subList(offset, offset + length));
187187
return new AggregationPath(subTokens);
188188
}
@@ -196,29 +196,38 @@ public double resolveValue(InternalAggregations aggregations) {
196196
assert path.hasNext();
197197
return aggregations.sortValue(path.next(), path);
198198
} catch (IllegalArgumentException e) {
199-
throw new IllegalArgumentException("Invalid aggregation order path [" + this + "]. " + e.getMessage(), e);
199+
throw new IllegalArgumentException("Invalid order path [" + this + "]. " + e.getMessage(), e);
200200
}
201201
}
202202

203203
/**
204-
* Resolves the {@linkplain Aggregator} pointed to by this path against
205-
* the given root {@linkplain Aggregator}.
204+
* Resolves the aggregator pointed by this path using the given root as a point of reference.
205+
*
206+
* @param root The point of reference of this path
207+
* @return The aggregator pointed by this path starting from the given aggregator as a point of reference
206208
*/
207209
public Aggregator resolveAggregator(Aggregator root) {
208-
Iterator<PathElement> path = pathElements.iterator();
209-
assert path.hasNext();
210-
return root.resolveSortPathOnValidAgg(path.next(), path);
210+
Aggregator aggregator = root;
211+
for (int i = 0; i < pathElements.size(); i++) {
212+
AggregationPath.PathElement token = pathElements.get(i);
213+
aggregator = ProfilingAggregator.unwrap(aggregator.subAggregator(token.name));
214+
assert (aggregator instanceof SingleBucketAggregator && i <= pathElements.size() - 1)
215+
|| (aggregator instanceof NumericMetricsAggregator && i == pathElements.size() - 1) :
216+
"this should be picked up before aggregation execution - on validate";
217+
}
218+
return aggregator;
211219
}
212220

213221
/**
214-
* Resolves the {@linkplain Aggregator} pointed to by the first element
215-
* of this path against the given root {@linkplain Aggregator}.
222+
* Resolves the topmost aggregator pointed by this path using the given root as a point of reference.
223+
*
224+
* @param root The point of reference of this path
225+
* @return The first child aggregator of the root pointed by this path
216226
*/
217227
public Aggregator resolveTopmostAggregator(Aggregator root) {
218228
AggregationPath.PathElement token = pathElements.get(0);
219-
// TODO both unwrap and subAggregator are only used here!
220229
Aggregator aggregator = ProfilingAggregator.unwrap(root.subAggregator(token.name));
221-
assert (aggregator instanceof SingleBucketAggregator)
230+
assert (aggregator instanceof SingleBucketAggregator )
222231
|| (aggregator instanceof NumericMetricsAggregator) : "this should be picked up before aggregation execution - on validate";
223232
return aggregator;
224233
}
@@ -230,10 +239,76 @@ public Aggregator resolveTopmostAggregator(Aggregator root) {
230239
* @throws AggregationExecutionException on validation error
231240
*/
232241
public void validate(Aggregator root) throws AggregationExecutionException {
233-
try {
234-
resolveAggregator(root).validateSortPathKey(lastPathElement().key);
235-
} catch (IllegalArgumentException e) {
236-
throw new AggregationExecutionException("Invalid aggregation order path [" + this + "]. " + e.getMessage(), e);
242+
Aggregator aggregator = root;
243+
for (int i = 0; i < pathElements.size(); i++) {
244+
String name = pathElements.get(i).name;
245+
aggregator = ProfilingAggregator.unwrap(aggregator.subAggregator(name));
246+
if (aggregator == null) {
247+
throw new AggregationExecutionException("Invalid aggregator order path [" + this + "]. The " +
248+
"provided aggregation [" + name + "] either does not exist, or is a pipeline aggregation " +
249+
"and cannot be used to sort the buckets.");
250+
}
251+
252+
if (i < pathElements.size() - 1) {
253+
254+
// we're in the middle of the path, so the aggregator can only be a single-bucket aggregator
255+
256+
if (!(aggregator instanceof SingleBucketAggregator)) {
257+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
258+
"]. Buckets can only be sorted on a sub-aggregator path " +
259+
"that is built out of zero or more single-bucket aggregations within the path and a final " +
260+
"single-bucket or a metrics aggregation at the path end. Sub-path [" +
261+
subPath(0, i + 1) + "] points to non single-bucket aggregation");
262+
}
263+
264+
if (pathElements.get(i).key != null) {
265+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
266+
"]. Buckets can only be sorted on a sub-aggregator path " +
267+
"that is built out of zero or more single-bucket aggregations within the path and a " +
268+
"final single-bucket or a metrics aggregation at the path end. Sub-path [" +
269+
subPath(0, i + 1) + "] points to non single-bucket aggregation");
270+
}
271+
}
272+
}
273+
boolean singleBucket = aggregator instanceof SingleBucketAggregator;
274+
if (!singleBucket && !(aggregator instanceof NumericMetricsAggregator)) {
275+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
276+
"]. Buckets can only be sorted on a sub-aggregator path " +
277+
"that is built out of zero or more single-bucket aggregations within the path and a final " +
278+
"single-bucket or a metrics aggregation at the path end.");
279+
}
280+
281+
AggregationPath.PathElement lastToken = lastPathElement();
282+
283+
if (singleBucket) {
284+
if (lastToken.key != null && !"doc_count".equals(lastToken.key)) {
285+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
286+
"]. Ordering on a single-bucket aggregation can only be done on its doc_count. " +
287+
"Either drop the key (a la \"" + lastToken.name + "\") or change it to \"doc_count\" (a la \"" + lastToken.name +
288+
".doc_count\")");
289+
}
290+
return; // perfectly valid to sort on single-bucket aggregation (will be sored on its doc_count)
291+
}
292+
293+
if (aggregator instanceof NumericMetricsAggregator.SingleValue) {
294+
if (lastToken.key != null && !"value".equals(lastToken.key)) {
295+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
296+
"]. Ordering on a single-value metrics aggregation can only be done on its value. " +
297+
"Either drop the key (a la \"" + lastToken.name + "\") or change it to \"value\" (a la \"" + lastToken.name +
298+
".value\")");
299+
}
300+
return; // perfectly valid to sort on single metric aggregation (will be sorted on its associated value)
301+
}
302+
303+
// the aggregator must be of a multi-value metrics type
304+
if (lastToken.key == null) {
305+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
306+
"]. When ordering on a multi-value metrics aggregation a metric name must be specified");
307+
}
308+
309+
if (!((NumericMetricsAggregator.MultiValue) aggregator).hasMetric(lastToken.key)) {
310+
throw new AggregationExecutionException("Invalid aggregation order path [" + this +
311+
"]. Unknown metric name [" + lastToken.key + "] on multi-value metrics aggregation [" + lastToken.name + "]");
237312
}
238313
}
239314

server/src/main/java/org/elasticsearch/search/profile/aggregation/ProfilingAggregator.java

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,10 @@
2424
import org.elasticsearch.search.aggregations.Aggregator;
2525
import org.elasticsearch.search.aggregations.InternalAggregation;
2626
import org.elasticsearch.search.aggregations.LeafBucketCollector;
27-
import org.elasticsearch.search.aggregations.support.AggregationPath.PathElement;
2827
import org.elasticsearch.search.internal.SearchContext;
2928
import org.elasticsearch.search.profile.Timer;
3029

3130
import java.io.IOException;
32-
import java.util.Iterator;
3331

3432
public class ProfilingAggregator extends Aggregator {
3533

@@ -72,16 +70,6 @@ public Aggregator subAggregator(String name) {
7270
return delegate.subAggregator(name);
7371
}
7472

75-
@Override
76-
public Aggregator resolveSortPath(PathElement next, Iterator<PathElement> path) {
77-
return delegate.resolveSortPath(next, path);
78-
}
79-
80-
@Override
81-
public void validateSortPathKey(String key) {
82-
delegate.validateSortPathKey(key);
83-
}
84-
8573
@Override
8674
public InternalAggregation buildAggregation(long bucket) throws IOException {
8775
Timer timer = profileBreakdown.getTimer(AggregationTimingType.BUILD_AGGREGATION);

server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1250,7 +1250,7 @@ public void testOrderByPipelineAggregation() throws Exception {
12501250

12511251
AggregationExecutionException e = expectThrows(AggregationExecutionException.class,
12521252
() -> createAggregator(termsAgg, indexSearcher, fieldType));
1253-
assertEquals("Invalid aggregation order path [script]. The provided aggregation [script] " +
1253+
assertEquals("Invalid aggregator order path [script]. The provided aggregation [script] " +
12541254
"either does not exist, or is a pipeline aggregation and cannot be used to sort the buckets.",
12551255
e.getMessage());
12561256
}

0 commit comments

Comments
 (0)