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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
