Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce execution_hint for Cardinality aggregation (#17312) - backport to 2.19 #17420

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

## [Unreleased 2.x]
### Added
- Add execution_hint to cardinality aggregator request - backport to 2.19 (#[17420](https://github.com/opensearch-project/OpenSearch/pull/17420))

### Dependencies
- Bump netty from 4.1.117.Final to 4.1.118.Final ([#17320](https://github.com/opensearch-project/OpenSearch/pull/17320))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@

private static final ParseField REHASH = new ParseField("rehash").withAllDeprecated("no replacement - values will always be rehashed");
public static final ParseField PRECISION_THRESHOLD_FIELD = new ParseField("precision_threshold");
public static final ParseField EXECUTION_HINT_FIELD = new ParseField("execution_hint");

public static final ObjectParser<CardinalityAggregationBuilder, String> PARSER = ObjectParser.fromBuilder(
NAME,
Expand All @@ -76,6 +77,7 @@
static {
ValuesSourceAggregationBuilder.declareFields(PARSER, true, false, false);
PARSER.declareLong(CardinalityAggregationBuilder::precisionThreshold, CardinalityAggregationBuilder.PRECISION_THRESHOLD_FIELD);
PARSER.declareString(CardinalityAggregationBuilder::executionHint, CardinalityAggregationBuilder.EXECUTION_HINT_FIELD);
PARSER.declareLong((b, v) -> {/*ignore*/}, REHASH);
}

Expand All @@ -85,6 +87,8 @@

private Long precisionThreshold = null;

private String executionHint = null;

public CardinalityAggregationBuilder(String name) {
super(name);
}
Expand All @@ -96,6 +100,7 @@
) {
super(clone, factoriesBuilder, metadata);
this.precisionThreshold = clone.precisionThreshold;
this.executionHint = clone.executionHint;
}

@Override
Expand All @@ -111,6 +116,9 @@
if (in.readBoolean()) {
precisionThreshold = in.readLong();
}
if (in.getVersion().onOrAfter(Version.V_2_19_1)) {
executionHint = in.readOptionalString();
}
}

@Override
Expand All @@ -125,6 +133,9 @@
if (hasPrecisionThreshold) {
out.writeLong(precisionThreshold);
}
if (out.getVersion().onOrAfter(Version.V_2_19_1)) {
out.writeOptionalString(executionHint);
}
}

@Override
Expand All @@ -146,13 +157,9 @@
return this;
}

/**
* Get the precision threshold. Higher values improve accuracy but also
* increase memory usage. Will return <code>null</code> if the
* precisionThreshold has not been set yet.
*/
public Long precisionThreshold() {
return precisionThreshold;
public CardinalityAggregationBuilder executionHint(String executionHint) {
this.executionHint = executionHint;
return this;
}

@Override
Expand All @@ -162,20 +169,32 @@
AggregatorFactory parent,
AggregatorFactories.Builder subFactoriesBuilder
) throws IOException {
return new CardinalityAggregatorFactory(name, config, precisionThreshold, queryShardContext, parent, subFactoriesBuilder, metadata);
return new CardinalityAggregatorFactory(
name,
config,
precisionThreshold,
queryShardContext,
parent,
subFactoriesBuilder,
metadata,
executionHint
);
}

@Override
public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException {
if (precisionThreshold != null) {
builder.field(PRECISION_THRESHOLD_FIELD.getPreferredName(), precisionThreshold);
}
if (executionHint != null) {
builder.field(EXECUTION_HINT_FIELD.getPreferredName(), executionHint);

Check warning on line 190 in server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregationBuilder.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregationBuilder.java#L190

Added line #L190 was not covered by tests
}
return builder;
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), precisionThreshold);
return Objects.hash(super.hashCode(), precisionThreshold, executionHint);
}

@Override
Expand All @@ -184,7 +203,7 @@
if (obj == null || getClass() != obj.getClass()) return false;
if (super.equals(obj) == false) return false;
CardinalityAggregationBuilder other = (CardinalityAggregationBuilder) obj;
return Objects.equals(precisionThreshold, other.precisionThreshold);
return Objects.equals(precisionThreshold, other.precisionThreshold) && Objects.equals(executionHint, other.executionHint);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public class CardinalityAggregator extends NumericMetricsAggregator.SingleValue

private static final Logger logger = LogManager.getLogger(CardinalityAggregator.class);

private final CardinalityAggregatorFactory.ExecutionMode executionMode;
private final int precision;
private final ValuesSource valuesSource;

Expand All @@ -113,14 +114,16 @@ public CardinalityAggregator(
int precision,
SearchContext context,
Aggregator parent,
Map<String, Object> metadata
Map<String, Object> metadata,
CardinalityAggregatorFactory.ExecutionMode executionMode
) throws IOException {
super(name, context, parent, metadata);
// TODO: Stop using nulls here
this.valuesSource = valuesSourceConfig.hasValues() ? valuesSourceConfig.getValuesSource() : null;
this.precision = precision;
this.counts = valuesSource == null ? null : new HyperLogLogPlusPlus(precision, context.bigArrays(), 1);
this.valuesSourceConfig = valuesSourceConfig;
this.executionMode = executionMode;
}

@Override
Expand Down Expand Up @@ -151,7 +154,11 @@ private Collector pickCollector(LeafReaderContext ctx) throws IOException {
if (maxOrd == 0) {
emptyCollectorsUsed++;
return new EmptyCollector();
} else {
} else if (executionMode == CardinalityAggregatorFactory.ExecutionMode.ORDINALS) { // Force OrdinalsCollector
ordinalsCollectorsUsed++;
collector = new OrdinalsCollector(counts, ordinalValues, context.bigArrays());
} else if (executionMode == null) {
// no hint provided, fall back to heuristics
final long ordinalsMemoryUsage = OrdinalsCollector.memoryOverhead(maxOrd);
final long countsMemoryUsage = HyperLogLogPlusPlus.memoryUsage(precision);
// only use ordinals if they don't increase memory usage by more than 25%
Expand All @@ -164,7 +171,7 @@ private Collector pickCollector(LeafReaderContext ctx) throws IOException {
}
}

if (collector == null) { // not able to build an OrdinalsCollector
if (collector == null) { // not able to build an OrdinalsCollector, or hint is direct
stringHashingCollectorsUsed++;
collector = new DirectCollector(counts, MurmurHash3Values.hash(valuesSource.bytesValues(ctx)));
}
Expand Down Expand Up @@ -480,7 +487,7 @@ public void close() {
*
* @opensearch.internal
*/
private static class DirectCollector extends Collector {
static class DirectCollector extends Collector {

private final MurmurHash3Values hashes;
private final HyperLogLogPlusPlus counts;
Expand Down Expand Up @@ -517,7 +524,7 @@ public void close() {
*
* @opensearch.internal
*/
private static class OrdinalsCollector extends Collector {
static class OrdinalsCollector extends Collector {

private static final long SHALLOW_FIXEDBITSET_SIZE = RamUsageEstimator.shallowSizeOfInstance(FixedBitSet.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.opensearch.search.internal.SearchContext;

import java.io.IOException;
import java.util.Locale;
import java.util.Map;

/**
Expand All @@ -53,6 +54,33 @@
*/
class CardinalityAggregatorFactory extends ValuesSourceAggregatorFactory {

/**
* Execution mode for cardinality agg
*
* @opensearch.internal
*/
public enum ExecutionMode {
DIRECT,
ORDINALS;

ExecutionMode() {}

public static ExecutionMode fromString(String value) {
try {
return ExecutionMode.valueOf(value.toUpperCase(Locale.ROOT));
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Unknown execution_hint: [" + value + "], expected any of [direct, ordinals]");
}
}

@Override
public String toString() {
return this.name().toLowerCase(Locale.ROOT);

Check warning on line 78 in server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorFactory.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorFactory.java#L78

Added line #L78 was not covered by tests
}
}

private final ExecutionMode executionMode;

private final Long precisionThreshold;

CardinalityAggregatorFactory(
Expand All @@ -62,10 +90,12 @@
QueryShardContext queryShardContext,
AggregatorFactory parent,
AggregatorFactories.Builder subFactoriesBuilder,
Map<String, Object> metadata
Map<String, Object> metadata,
String executionHint
) throws IOException {
super(name, config, queryShardContext, parent, subFactoriesBuilder, metadata);
this.precisionThreshold = precisionThreshold;
this.executionMode = executionHint == null ? null : ExecutionMode.fromString(executionHint);
}

public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
Expand All @@ -74,7 +104,7 @@

@Override
protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map<String, Object> metadata) throws IOException {
return new CardinalityAggregator(name, config, precision(), searchContext, parent, metadata);
return new CardinalityAggregator(name, config, precision(), searchContext, parent, metadata, executionMode);

Check warning on line 107 in server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorFactory.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorFactory.java#L107

Added line #L107 was not covered by tests
}

@Override
Expand All @@ -86,7 +116,7 @@
) throws IOException {
return queryShardContext.getValuesSourceRegistry()
.getAggregator(CardinalityAggregationBuilder.REGISTRY_KEY, config)
.build(name, config, precision(), searchContext, parent, metadata);
.build(name, config, precision(), searchContext, parent, metadata, executionMode);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ Aggregator build(
int precision,
SearchContext context,
Aggregator parent,
Map<String, Object> metadata
Map<String, Object> metadata,
CardinalityAggregatorFactory.ExecutionMode executionMode
) throws IOException;
}
Loading
Loading