Skip to content

Commit c95a15a

Browse files
committed
Deprecate global_ordinals_hash and global_ordinals_low_cardinality (#26173)
* Deprecate global_ordinals_hash and global_ordinals_low_cardinality This change deprecates the `global_ordinals_hash` and `global_ordinals_low_cardinality` and makes the `global_ordinals` execution hint choose internally if global ords should be remapped or use the segment ord directly. These hints are too sensitive and expert to be exposed and we should be able to take the right decision internally based on the agg tree.
1 parent c77b13d commit c95a15a

File tree

7 files changed

+126
-244
lines changed

7 files changed

+126
-244
lines changed

core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java

Lines changed: 28 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
import org.apache.lucene.util.BytesRef;
3131
import org.elasticsearch.common.ParseField;
3232
import org.elasticsearch.common.lease.Releasable;
33+
import org.elasticsearch.common.logging.DeprecationLogger;
34+
import org.elasticsearch.common.logging.Loggers;
3335
import org.elasticsearch.common.lucene.index.FilterableTermsEnum;
3436
import org.elasticsearch.common.lucene.index.FreqTermsEnum;
3537
import org.elasticsearch.index.mapper.MappedFieldType;
@@ -53,12 +55,12 @@
5355
import org.elasticsearch.search.internal.SearchContext;
5456

5557
import java.io.IOException;
56-
import java.util.Arrays;
5758
import java.util.List;
5859
import java.util.Map;
5960

6061
public class SignificantTermsAggregatorFactory extends ValuesSourceAggregatorFactory<ValuesSource, SignificantTermsAggregatorFactory>
6162
implements Releasable {
63+
private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(SignificantTermsAggregatorFactory.class));
6264

6365
private final IncludeExclude includeExclude;
6466
private final String executionHint;
@@ -202,17 +204,13 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare
202204
if (valuesSource instanceof ValuesSource.Bytes) {
203205
ExecutionMode execution = null;
204206
if (executionHint != null) {
205-
execution = ExecutionMode.fromString(executionHint);
207+
execution = ExecutionMode.fromString(executionHint, DEPRECATION_LOGGER);
206208
}
207-
if (!(valuesSource instanceof ValuesSource.Bytes.WithOrdinals)) {
209+
if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals == false) {
208210
execution = ExecutionMode.MAP;
209211
}
210212
if (execution == null) {
211-
if (Aggregator.descendsFromBucketAggregator(parent)) {
212-
execution = ExecutionMode.GLOBAL_ORDINALS_HASH;
213-
} else {
214-
execution = ExecutionMode.GLOBAL_ORDINALS;
215-
}
213+
execution = ExecutionMode.GLOBAL_ORDINALS;
216214
}
217215
assert execution != null;
218216

@@ -291,44 +289,35 @@ Aggregator create(String name,
291289
Map<String, Object> metaData) throws IOException {
292290

293291
final IncludeExclude.OrdinalsFilter filter = includeExclude == null ? null : includeExclude.convertToOrdinalsFilter(format);
292+
boolean remapGlobalOrd = true;
293+
if (Aggregator.descendsFromBucketAggregator(parent) == false &&
294+
factories == AggregatorFactories.EMPTY &&
295+
includeExclude == null) {
296+
/**
297+
* We don't need to remap global ords iff this aggregator:
298+
* - is not a child of a bucket aggregator AND
299+
* - has no include/exclude rules AND
300+
* - has no sub-aggregator
301+
**/
302+
remapGlobalOrd = false;
303+
}
294304
return new GlobalOrdinalsSignificantTermsAggregator(name, factories,
295305
(ValuesSource.Bytes.WithOrdinals.FieldData) valuesSource, format, bucketCountThresholds, filter,
296-
aggregationContext, parent, false, significanceHeuristic, termsAggregatorFactory, pipelineAggregators, metaData);
297-
298-
}
299-
300-
},
301-
GLOBAL_ORDINALS_HASH(new ParseField("global_ordinals_hash")) {
302-
303-
@Override
304-
Aggregator create(String name,
305-
AggregatorFactories factories,
306-
ValuesSource valuesSource,
307-
DocValueFormat format,
308-
TermsAggregator.BucketCountThresholds bucketCountThresholds,
309-
IncludeExclude includeExclude,
310-
SearchContext aggregationContext,
311-
Aggregator parent,
312-
SignificanceHeuristic significanceHeuristic,
313-
SignificantTermsAggregatorFactory termsAggregatorFactory,
314-
List<PipelineAggregator> pipelineAggregators,
315-
Map<String, Object> metaData) throws IOException {
316-
317-
final IncludeExclude.OrdinalsFilter filter = includeExclude == null ? null : includeExclude.convertToOrdinalsFilter(format);
318-
return new GlobalOrdinalsSignificantTermsAggregator(name, factories,
319-
(ValuesSource.Bytes.WithOrdinals.FieldData) valuesSource, format, bucketCountThresholds, filter, aggregationContext, parent,
320-
true, significanceHeuristic, termsAggregatorFactory, pipelineAggregators, metaData);
306+
aggregationContext, parent, remapGlobalOrd, significanceHeuristic, termsAggregatorFactory, pipelineAggregators, metaData);
321307

322308
}
323309
};
324310

325-
public static ExecutionMode fromString(String value) {
326-
for (ExecutionMode mode : values()) {
327-
if (mode.parseField.match(value)) {
328-
return mode;
329-
}
311+
public static ExecutionMode fromString(String value, final DeprecationLogger deprecationLogger) {
312+
if ("global_ordinals".equals(value)) {
313+
return GLOBAL_ORDINALS;
314+
} else if ("global_ordinals_hash".equals(value)) {
315+
deprecationLogger.deprecated("global_ordinals_hash is deprecated. Please use [global_ordinals] instead.");
316+
return GLOBAL_ORDINALS;
317+
} else if ("map".equals(value)) {
318+
return MAP;
330319
}
331-
throw new IllegalArgumentException("Unknown `execution_hint`: [" + value + "], expected any of " + Arrays.toString(values()));
320+
throw new IllegalArgumentException("Unknown `execution_hint`: [" + value + "], expected any of [map, global_ordinals]");
332321
}
333322

334323
private final ParseField parseField;

core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -78,20 +78,20 @@ public interface GlobalOrdLookupFunction {
7878
}
7979

8080
public GlobalOrdinalsStringTermsAggregator(String name, AggregatorFactories factories,
81-
ValuesSource.Bytes.WithOrdinals valuesSource,
82-
BucketOrder order,
83-
DocValueFormat format,
84-
BucketCountThresholds bucketCountThresholds,
85-
IncludeExclude.OrdinalsFilter includeExclude,
86-
SearchContext context,
87-
Aggregator parent,
88-
boolean forceRemapGlobalOrds,
89-
SubAggCollectionMode collectionMode,
90-
boolean showTermDocCountError,
91-
List<PipelineAggregator> pipelineAggregators,
92-
Map<String, Object> metaData) throws IOException {
81+
ValuesSource.Bytes.WithOrdinals valuesSource,
82+
BucketOrder order,
83+
DocValueFormat format,
84+
BucketCountThresholds bucketCountThresholds,
85+
IncludeExclude.OrdinalsFilter includeExclude,
86+
SearchContext context,
87+
Aggregator parent,
88+
boolean remapGlobalOrds,
89+
SubAggCollectionMode collectionMode,
90+
boolean showTermDocCountError,
91+
List<PipelineAggregator> pipelineAggregators,
92+
Map<String, Object> metaData) throws IOException {
9393
super(name, factories, context, parent, order, format, bucketCountThresholds, collectionMode, showTermDocCountError,
94-
pipelineAggregators, metaData);
94+
pipelineAggregators, metaData);
9595
this.valuesSource = valuesSource;
9696
this.includeExclude = includeExclude;
9797
final IndexReader reader = context.searcher().getIndexReader();
@@ -100,18 +100,9 @@ public GlobalOrdinalsStringTermsAggregator(String name, AggregatorFactories fact
100100
this.valueCount = values.getValueCount();
101101
this.lookupGlobalOrd = values::lookupOrd;
102102
this.acceptedGlobalOrdinals = includeExclude != null ? includeExclude.acceptedGlobalOrdinals(values) : null;
103-
104-
/**
105-
* Remap global ords to dense bucket ordinals if any sub-aggregator cannot be deferred.
106-
* Sub-aggregators expect dense buckets and allocate memories based on this assumption.
107-
* Deferred aggregators are safe because the selected ordinals are remapped when the buckets
108-
* are replayed.
109-
*/
110-
boolean remapGlobalOrds = forceRemapGlobalOrds || Arrays.stream(subAggregators).anyMatch((a) -> shouldDefer(a) == false);
111103
this.bucketOrds = remapGlobalOrds ? new LongHash(1, context.bigArrays()) : null;
112104
}
113105

114-
115106
boolean remapGlobalOrds() {
116107
return bucketOrds != null;
117108
}

0 commit comments

Comments
 (0)