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.

Sign up for GitHub

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

Added support for accurate distinct count on attributes through config #178

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Added support for accurate distinct count on attributes through config #178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40c8636
Added support for accurate distinct count on attributes through config
singhalprerana Dec 15, 2022
f51ccb7
Added generic overrides map
singhalprerana Jan 10, 2023
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
14 changes: 7 additions & 7 deletions .../main/java/org/hypertrace/core/query/service/pinot/converters/PinotFunctionConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public String convert(
// and reasonably accurate, so support selecting the implementation to use
return this.functionToString(this.toPinotPercentile(function), argumentConverter);
case QUERY_FUNCTION_DISTINCTCOUNT:
return this.functionToString(this.toPinotDistinctCount(function), argumentConverter);
return this.functionToStringForDistinctCount(function, argumentConverter);
case QUERY_FUNCTION_CONCAT:
return this.functionToString(this.toPinotConcat(function), argumentConverter);
case QUERY_FUNCTION_AVGRATE:
Expand Down Expand Up @@ -72,6 +72,12 @@ private String functionToString(
return function.getFunctionName() + "(" + argumentString + ")";
}

private String functionToStringForDistinctCount(
Function function, java.util.function.Function<Expression, String> argumentConverter) {
String columnName = argumentConverter.apply(function.getArgumentsList().get(0));
return this.config.getDistinctCountFunction(columnName) + "(" + columnName + ")";
}

private String functionToStringForAvgRate(
Function function,
java.util.function.Function<Expression, String> argumentConverter,
Expand Down Expand Up @@ -119,12 +125,6 @@ private Function toPinotConcat(Function function) {
return Function.newBuilder(function).setFunctionName(PINOT_CONCAT_FUNCTION).build();
}

private Function toPinotDistinctCount(Function function) {
return Function.newBuilder(function)
.setFunctionName(this.config.getDistinctCountAggregationFunction())
.build();
}

private boolean isHardcodedPercentile(Function function) {
String functionName = function.getFunctionName().toUpperCase();
return functionName.startsWith(QUERY_FUNCTION_PERCENTILE)
Expand Down
27 changes: 24 additions & 3 deletions ...java/org/hypertrace/core/query/service/pinot/converters/PinotFunctionConverterConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,30 @@
package org.hypertrace.core.query.service.pinot.converters;

import com.google.common.collect.ImmutableSet;
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
import java.util.Collections;
import java.util.Set;
import javax.annotation.Nonnull;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Value;

@Value
@AllArgsConstructor
@AllArgsConstructor(access = AccessLevel.PACKAGE)
public class PinotFunctionConverterConfig {

private static final String PERCENTILE_AGGREGATION_FUNCTION_CONFIG = "percentileAggFunction";
private static final String DISTINCT_COUNT_AGGREGATION_FUNCTION_CONFIG =
"distinctCountAggFunction";
private static final String ARGS_FOR_ACCURATE_DISTINCT_COUNT_AGG_CONFIG =
singhalprerana marked this conversation as resolved.
Show resolved Hide resolved
"argsForAccurateDistinctCountAgg";
private static final String DEFAULT_PERCENTILE_AGGREGATION_FUNCTION = "PERCENTILETDIGEST";
private static final String DEFAULT_DISTINCT_COUNT_AGGREGATION_FUNCTION = "DISTINCTCOUNT";
private static final String ACCURATE_DISTINCT_COUNT_AGGREGATION_FUNCTION = "DISTINCTCOUNT";

String percentileAggregationFunction;
String distinctCountAggregationFunction;
@Nonnull Set<String> argsForAccurateDistinctCountAgg;

public PinotFunctionConverterConfig(Config config) {
if (config.hasPath(PERCENTILE_AGGREGATION_FUNCTION_CONFIG)) {
Expand All @@ -28,11 +36,24 @@ public PinotFunctionConverterConfig(Config config) {
this.distinctCountAggregationFunction =
config.getString(DISTINCT_COUNT_AGGREGATION_FUNCTION_CONFIG);
} else {
this.distinctCountAggregationFunction = DEFAULT_DISTINCT_COUNT_AGGREGATION_FUNCTION;
this.distinctCountAggregationFunction = ACCURATE_DISTINCT_COUNT_AGGREGATION_FUNCTION;
}
if (config.hasPath(ARGS_FOR_ACCURATE_DISTINCT_COUNT_AGG_CONFIG)) {
this.argsForAccurateDistinctCountAgg =
ImmutableSet.copyOf(config.getStringList(ARGS_FOR_ACCURATE_DISTINCT_COUNT_AGG_CONFIG));
} else {
this.argsForAccurateDistinctCountAgg = Collections.emptySet();
}
}

public PinotFunctionConverterConfig() {
this(ConfigFactory.empty());
}

public String getDistinctCountFunction(String arg) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit rename arg to what the string is supposed to represent (columnName)

if (argsForAccurateDistinctCountAgg.contains(arg)) {
return ACCURATE_DISTINCT_COUNT_AGGREGATION_FUNCTION;
}
return distinctCountAggregationFunction;
}
}
36 changes: 28 additions & 8 deletions ...t/java/org/hypertrace/core/query/service/pinot/converters/PinotFunctionConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@

import java.time.Duration;
import java.util.Arrays;
import java.util.Collections;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.hypertrace.core.query.service.ExecutionContext;
import org.hypertrace.core.query.service.api.Expression;
Expand Down Expand Up @@ -104,7 +106,8 @@ void acceptsCustomPercentileFunctions() {

assertEquals(
expected,
new PinotFunctionConverter(new PinotFunctionConverterConfig("CUSTOMPERCENTILE", null))
new PinotFunctionConverter(
new PinotFunctionConverterConfig("CUSTOMPERCENTILE", null, Collections.emptySet()))
.convert(mockingExecutionContext, percentileFunction, this.mockArgumentConverter));
}

Expand Down Expand Up @@ -233,25 +236,42 @@ void convertAvgRateFunction() {

@Test
void convertsDistinctCountFunction() {
Expression column = createColumnExpression("foo").build();
Expression column1 = createColumnExpression("foo").build();
Expression column2 = createColumnExpression("bar").build();

when(this.mockArgumentConverter.apply(column)).thenReturn("foo");
when(this.mockArgumentConverter.apply(column1)).thenReturn("foo");
when(this.mockArgumentConverter.apply(column2)).thenReturn("bar");

assertEquals(
"DISTINCTCOUNT(foo)",
new PinotFunctionConverter()
.convert(
mockingExecutionContext,
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column.toBuilder()),
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column1.toBuilder()),
this.mockArgumentConverter));

assertEquals(
"CUSTOM_DC(foo)",
new PinotFunctionConverter(new PinotFunctionConverterConfig(null, "CUSTOM_DC"))
"DISTINCTCOUNT(bar)",
new PinotFunctionConverter()
.convert(
mockingExecutionContext,
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column.toBuilder()),
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column2.toBuilder()),
this.mockArgumentConverter));

PinotFunctionConverter converter =
new PinotFunctionConverter(
new PinotFunctionConverterConfig(null, "CUSTOM_DC", Set.of("bar")));
assertEquals(
"CUSTOM_DC(foo)",
converter.convert(
mockingExecutionContext,
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column1.toBuilder()),
this.mockArgumentConverter));
assertEquals(
"DISTINCTCOUNT(bar)",
converter.convert(
mockingExecutionContext,
buildFunction(QUERY_FUNCTION_DISTINCTCOUNT, column2.toBuilder()),
this.mockArgumentConverter));
}

@Test
Expand Down