You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2024/07/08 05:00:18 UTC
Re: [PR] Preemptive restriction for queries with approximate count distinct on complex columns of unsupported type (druid)
LakshSingla commented on code in PR #16682:
URL: https://github.com/apache/druid/pull/16682#discussion_r1667970357
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(type.getComplexTypeName(), "HLLSketchBuild")))) {
+ throw new UOE("Using aggregation type %s is not supported for %s column. "
Review Comment:
We don't use UOE nowadays. DruidException is used for better error messages. Let's use that instead.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(type.getComplexTypeName(), "HLLSketchBuild")))) {
+ throw new UOE("Using aggregation type %s is not supported for %s column. "
+ + "Use a different aggregator type and run the query again.",
+ getIntermediateType().getComplexTypeName(),
Review Comment:
From the error message, this is suppposed to be an "aggregation type". This is the complex column type name, not the aggregation type.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactoryTest.java:
##########
@@ -66,6 +76,11 @@ public void setUp()
SHOULD_FINALIZE,
!ROUND
);
+
+ EasyMock.expect(metricFactory.getColumnCapabilities(EasyMock.anyString())).andReturn(capabilities).anyTimes();
+ EasyMock.expect(vectorFactory.getColumnCapabilities(EasyMock.anyString())).andReturn(capabilities).anyTimes();
+ EasyMock.expect(capabilities.toColumnType()).andReturn(ColumnType.NESTED_DATA).anyTimes();
+ EasyMock.replay(metricFactory, vectorFactory, capabilities);
Review Comment:
Let's not mock everything. Please reuse existing classes instead of mocking the selectors
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregatorFactoryTest.java:
##########
@@ -46,6 +51,22 @@ public class SketchAggregatorFactoryTest
private static final SketchMergeAggregatorFactory AGGREGATOR_32768 =
new SketchMergeAggregatorFactory("x", "x", 32768, null, false, null);
+ private final ColumnSelectorFactory metricFactory = EasyMock.mock(ColumnSelectorFactory.class);
+ private final VectorColumnSelectorFactory vectorFactory = EasyMock.mock(VectorColumnSelectorFactory.class);
+ private final ColumnCapabilities capabilities = EasyMock.mock(ColumnCapabilities.class);
+
+ @Before
+ public void setup()
+ {
+ EasyMock.expect(metricFactory.getColumnCapabilities(EasyMock.anyString())).andReturn(capabilities).anyTimes();
+ EasyMock.expect(vectorFactory.getColumnCapabilities(EasyMock.anyString())).andReturn(capabilities).anyTimes();
+ EasyMock.expect(capabilities.toColumnType()).andReturn(ColumnType.NESTED_DATA).anyTimes();
+ EasyMock.expect(capabilities.isArray()).andReturn(false).anyTimes();
+ EasyMock.expect(capabilities.is(EasyMock.eq(ValueType.COMPLEX))).andReturn(true).anyTimes();
+ EasyMock.expect(capabilities.asTypeString()).andReturn(ColumnType.NESTED_DATA.asTypeString()).anyTimes();
+ EasyMock.replay(metricFactory, vectorFactory, capabilities);
+ }
Review Comment:
Let's actively try to avoid mocks
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
Review Comment:
Conditional is too complex, let's break it for easy reading.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
Review Comment:
Please refrain from using 'HllSketch' directly as a literal. There should be some variable that is either equal to 'HllSketch'. Let's use that instead.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
Review Comment:
Please add a Javadoc here as to what this method does, and what those validations are and why.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchApproxCountDistinctSqlAggregator.java:
##########
@@ -21,28 +21,64 @@
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.type.CastedLiteralOperandTypeCheckers;
import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlSingleOperandTypeChecker;
import org.apache.calcite.sql.type.SqlTypeFamily;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.druid.java.util.common.StringEncoding;
+import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.datasketches.hll.HllSketchBuildAggregatorFactory;
+import org.apache.druid.query.aggregation.datasketches.hll.HllSketchMergeAggregatorFactory;
import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.table.RowSignatures;
import java.util.Collections;
public class HllSketchApproxCountDistinctSqlAggregator extends HllSketchBaseSqlAggregator implements SqlAggregator
{
public static final String NAME = "APPROX_COUNT_DISTINCT_DS_HLL";
+
+ private static final SqlSingleOperandTypeChecker COLUMN_ALLOWED_TYPES = OperandTypes.or(
Review Comment:
nit: Variable name is unclear. We should use something like `AGGREGATED_COLUMN_TYPE_CHECKER`, and add a javadoc as to what types are expected.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java:
##########
Review Comment:
Given that the `HllSketchApproxCountDistinctSqlAggregator.java` type checks the input column type when matching the function signature, are the changes in this class relevant?
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(type.getComplexTypeName(), "HLLSketchBuild")))) {
+ throw new UOE("Using aggregation type %s is not supported for %s column. "
+ + "Use a different aggregator type and run the query again.",
+ getIntermediateType().getComplexTypeName(),
+ type);
Review Comment:
Incorrect formatting
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchApproxCountDistinctSqlAggregator.java:
##########
@@ -21,27 +21,51 @@
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.type.CastedLiteralOperandTypeCheckers;
import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlSingleOperandTypeChecker;
import org.apache.calcite.sql.type.SqlTypeFamily;
import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.datasketches.theta.SketchModule;
import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.table.RowSignatures;
import java.util.Collections;
public class ThetaSketchApproxCountDistinctSqlAggregator extends ThetaSketchBaseSqlAggregator implements SqlAggregator
{
public static final String NAME = "APPROX_COUNT_DISTINCT_DS_THETA";
+
+ private static final SqlSingleOperandTypeChecker COLUMN_ALLOWED_TYPES = OperandTypes.or(
Review Comment:
Same comment as above - `COLUMN_ALLOWED_TYPES` is a weird variable name for following reasons:
1. It doesn't make much sense if read without context
2. It is not a type, but typeChecker.
Let's rename it more appropriately.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchApproxCountDistinctSqlAggregator.java:
##########
@@ -21,28 +21,64 @@
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.type.CastedLiteralOperandTypeCheckers;
import org.apache.calcite.sql.type.InferTypes;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.SqlSingleOperandTypeChecker;
import org.apache.calcite.sql.type.SqlTypeFamily;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.druid.java.util.common.StringEncoding;
+import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.aggregation.AggregatorFactory;
+import org.apache.druid.query.aggregation.datasketches.hll.HllSketchBuildAggregatorFactory;
+import org.apache.druid.query.aggregation.datasketches.hll.HllSketchMergeAggregatorFactory;
import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.table.RowSignatures;
import java.util.Collections;
public class HllSketchApproxCountDistinctSqlAggregator extends HllSketchBaseSqlAggregator implements SqlAggregator
{
public static final String NAME = "APPROX_COUNT_DISTINCT_DS_HLL";
+
+ private static final SqlSingleOperandTypeChecker COLUMN_ALLOWED_TYPES = OperandTypes.or(
+ OperandTypes.STRING,
+ OperandTypes.NUMERIC,
+ RowSignatures.complexTypeChecker(HllSketchMergeAggregatorFactory.TYPE),
+ RowSignatures.complexTypeChecker(HllSketchBuildAggregatorFactory.TYPE)
+ );
+
private static final SqlAggFunction FUNCTION_INSTANCE =
OperatorConversions.aggregatorBuilder(NAME)
- .operandNames("column", "lgK", "tgtHllType")
- .operandTypes(SqlTypeFamily.ANY, SqlTypeFamily.NUMERIC, SqlTypeFamily.STRING)
.operandTypeInference(InferTypes.VARCHAR_1024)
- .requiredOperandCount(1)
- .literalOperands(1, 2)
+ .operandTypeChecker(
+ OperandTypes.or(
+ // APPROX_COUNT_DISTINCT_DS_HLL(column)
+ OperandTypes.and(COLUMN_ALLOWED_TYPES, OperandTypes.family(SqlTypeFamily.ANY)),
+ // APPROX_COUNT_DISTINCT_DS_HLL(column, lgk)
+ OperandTypes.and(
+ OperandTypes.sequence(
+ StringUtils.format("'%s(column, lgk)'", NAME),
+ COLUMN_ALLOWED_TYPES,
+ CastedLiteralOperandTypeCheckers.POSITIVE_INTEGER_LITERAL
Review Comment:
Nice addition on making it a positive literal 🎸
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java:
##########
@@ -154,19 +160,23 @@ public Aggregation toDruidAggregation(
}
if (inputType.is(ValueType.COMPLEX)) {
- aggregatorFactory = new HllSketchMergeAggregatorFactory(
- aggregatorName,
- dimensionSpec.getOutputName(),
- logK,
- tgtHllType,
-
- // For HllSketchMergeAggregatorFactory, stringEncoding is only advisory to aid in detection of mismatched
- // merges. It does not affect the results of the aggregator. At this point in the code, we do not know what
- // the input encoding of the original sketches was, so we set it to the default.
- HllSketchAggregatorFactory.DEFAULT_STRING_ENCODING,
- finalizeSketch || SketchQueryContext.isFinalizeOuterSketches(plannerContext),
- ROUND
- );
+ if (HllSketchMergeAggregatorFactory.TYPE.equals(inputType) ||
+ Objects.equals(inputType.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(inputType.getComplexTypeName(), "HLLSketchBuild")) {
Review Comment:
Let's refrain from using string variables here and replace them with already present static strings.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregatorFactory.java:
##########
@@ -104,20 +99,37 @@ public AggregatorAndSize factorizeWithSize(ColumnSelectorFactory metricFactory)
@Override
public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory)
{
- ColumnCapabilities capabilities = metricFactory.getColumnCapabilities(fieldName);
- if (capabilities != null && capabilities.isArray()) {
- throw InvalidInput.exception("ARRAY types are not supported for theta sketch");
- }
+ validateInputs(metricFactory.getColumnCapabilities(fieldName));
BaseObjectColumnValueSelector selector = metricFactory.makeColumnValueSelector(fieldName);
return new SketchBufferAggregator(selector, size, getMaxIntermediateSizeWithNulls());
}
@Override
public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFactory)
{
+ validateInputs(selectorFactory.getColumnCapabilities(fieldName));
return new SketchVectorAggregator(selectorFactory, fieldName, size, getMaxIntermediateSizeWithNulls());
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ if (capabilities.isArray() || (capabilities.is(ValueType.COMPLEX) && !(
+ SketchModule.THETA_SKETCH_TYPE.equals(capabilities.toColumnType()) ||
+ SketchModule.MERGE_TYPE.equals(capabilities.toColumnType()) ||
+ SketchModule.BUILD_TYPE.equals(capabilities.toColumnType())))
+ ) {
+ throw new ISE(
+ "Invalid input [%s] of type [%s] for [%s] aggregator [%s]",
Review Comment:
Checkout extrapolations in the error message style guide. Third extrapolation is incorrect syntactically.
Also, the error message isn't correct, especially the third one. Aggregator doesn't have a type if I am not mistaken. We can simply use
```suggestion
"Invalid input [%s] of type [%s] for aggregator [%s]",
```
Also, let's reword it as "Not supported", instead of invalid input.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregatorFactory.java:
##########
@@ -104,20 +99,37 @@ public AggregatorAndSize factorizeWithSize(ColumnSelectorFactory metricFactory)
@Override
public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory)
{
- ColumnCapabilities capabilities = metricFactory.getColumnCapabilities(fieldName);
- if (capabilities != null && capabilities.isArray()) {
- throw InvalidInput.exception("ARRAY types are not supported for theta sketch");
- }
+ validateInputs(metricFactory.getColumnCapabilities(fieldName));
BaseObjectColumnValueSelector selector = metricFactory.makeColumnValueSelector(fieldName);
return new SketchBufferAggregator(selector, size, getMaxIntermediateSizeWithNulls());
}
@Override
public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFactory)
{
+ validateInputs(selectorFactory.getColumnCapabilities(fieldName));
return new SketchVectorAggregator(selectorFactory, fieldName, size, getMaxIntermediateSizeWithNulls());
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ if (capabilities.isArray() || (capabilities.is(ValueType.COMPLEX) && !(
+ SketchModule.THETA_SKETCH_TYPE.equals(capabilities.toColumnType()) ||
+ SketchModule.MERGE_TYPE.equals(capabilities.toColumnType()) ||
+ SketchModule.BUILD_TYPE.equals(capabilities.toColumnType())))
+ ) {
+ throw new ISE(
Review Comment:
Let's use DruidException instead of ISE.
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java:
##########
@@ -180,6 +190,11 @@ public Aggregation toDruidAggregation(
}
}
+ if (aggregatorFactory == null) {
+ plannerContext.setPlanningError("Using APPROX_COUNT_DISTINCT() or enabling approximation with COUNT(DISTINCT) is not supported for %s column. You can disable approximation and use COUNT(DISTINCT %s) and run the query again.", columnArg.getDruidType(), columnArg.getSimpleExtraction().getColumn());
Review Comment:
1. Please format it according to the dev guide.
2. `columnArg.getSimpleExtraction().getColumn()` might not be a one to one mapping to the column name that you are expecting. It can also throw an NPE.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSqlAggregatorTest.java:
##########
@@ -300,6 +307,15 @@ public SpecificSegmentsQuerySegmentWalker createQuerySegmentWalker(
.size(0)
.build(),
index
+ ).add(
+ DataSegment.builder()
+ .dataSource(CalciteTests.WIKIPEDIA_FIRST_LAST)
+ .interval(Intervals.of("2015-09-12/2015-09-13"))
+ .version("1")
+ .shardSpec(new NumberedShardSpec(0, 0))
+ .size(0)
+ .build(),
+ TestDataBuilder.makeWikipediaIndexWithAggregation(tempDirProducer.newTempFolder())
Review Comment:
Instead of this, we can use the parent method to register the datasource:
```java
public SpecificSegmentsQuerySegmentWalker createQuerySegmentWalker(
final QueryRunnerFactoryConglomerate conglomerate,
final JoinableFactoryWrapper joinableFactory,
final Injector injector
)
{
SpecificSegmentsQuerySegmentWalker walker = super.createQuerySegmentWalker(conglomerate...);
// Do the remaining processing that's done earlier here
}
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchBaseSqlAggregator.java:
##########
Review Comment:
Similar comment as above, are these changes required given that we are disallowing incorrect types in the function signature itself?
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSqlAggregatorTest.java:
##########
@@ -508,6 +524,33 @@ public void testApproxCountDistinctHllSketchIsRounded()
);
}
+ @Test
+ public void testApproxCountDistinctOnUnsupportedComplexColumn()
+ {
+ try {
+ testQuery("SELECT COUNT(distinct double_first_added) FROM druid.wikipedia_first_last", ImmutableList.of(), ImmutableList.of());
+ Assert.fail("query planning should fail");
+ }
+ catch (DruidException e) {
+ Assert.assertEquals("Query could not be planned. A possible reason is [Using APPROX_COUNT_DISTINCT() or enabling approximation "
Review Comment:
Why does the SQL layer not catch it? Is it because we are bypassing the type check when using `COUNT(DiSTINCT ...)`?
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(type.getComplexTypeName(), "HLLSketchBuild")))) {
+ throw new UOE("Using aggregation type %s is not supported for %s column. "
Review Comment:
Please format the message according to [style guide](https://github.com/apache/druid/blob/master/dev/style-conventions.md#message-formatting-for-logs-and-exceptions). For example, converting the error message as is means:
```suggestion
throw new UOE("aggregation type [%s] is not supported for complex columns with type [%s] "
```
##########
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactory.java:
##########
@@ -142,6 +151,22 @@ public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFact
);
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ final ColumnType type = capabilities.toColumnType();
+ if (!ColumnType.UNKNOWN_COMPLEX.equals(type) && !TYPE.equals(type) &&
+ !(ValueType.COMPLEX.equals(type.getType()) &&
+ (Objects.equals(type.getComplexTypeName(), "HLLSketch") ||
+ Objects.equals(type.getComplexTypeName(), "HLLSketchBuild")))) {
+ throw new UOE("Using aggregation type %s is not supported for %s column. "
Review Comment:
There are a couple of things that should be changed regarding the error message:
1. "aggregatorType" isn't a thing afaik. We should simply use aggregator
2. The message isn't actionable enough - "Use a different aggregator type and run the query again". What different aggregators can be used by the user? If we cannot glean it from the given type, then let's just remove this portion of the message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org