clintropolis commented on a change in pull request #12131:
URL: https://github.com/apache/druid/pull/12131#discussion_r780531388
##########
File path:
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildAggregatorFactory.java
##########
@@ -114,4 +118,19 @@ public int getMaxIntermediateSize()
return HllSketch.getMaxUpdatableSerializationBytes(getLgK(),
TgtHllType.valueOf(getTgtHllType()));
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
Review comment:
nit: can use `Types.is(capabilities, ValueType.COMPLEX)` to get rid of
outer if statement since it null checks
##########
File path:
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildAggregatorFactory.java
##########
@@ -71,6 +74,7 @@ protected byte getCacheTypeId()
public Aggregator factorize(final ColumnSelectorFactory
columnSelectorFactory)
{
final ColumnValueSelector<Object> selector =
columnSelectorFactory.makeColumnValueSelector(getFieldName());
+
validateInputs(columnSelectorFactory.getColumnCapabilities(getFieldName()));
Review comment:
`factorizeBuffer` and `factorizeVector` should also validate their inputs
also please add tests for this stuff
##########
File path:
extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildAggregatorFactory.java
##########
@@ -114,4 +118,19 @@ public int getMaxIntermediateSize()
return HllSketch.getMaxUpdatableSerializationBytes(getLgK(),
TgtHllType.valueOf(getTgtHllType()));
}
+ private void validateInputs(@Nullable ColumnCapabilities capabilities)
+ {
+ if (capabilities != null) {
+ if (capabilities.is(ValueType.COMPLEX)) {
+ throw new ISE(
+ "Invalid input type [%s] in ingestion for metric type %s, in
aggregate %s for field name %s",
+ capabilities.asTypeString(),
+ HllSketchModule.BUILD_TYPE_NAME,
+ getName(),
+ getFieldName()
+ );
Review comment:
this method is used at both query and ingestion time, and shouldn't
mention ingestion
```suggestion
throw new ISE(
"Invalid input [%s] of type [%s] for [%s] aggregator [%s]",
getFieldName(),
capabilities.asTypeString(),
HllSketchModule.BUILD_TYPE_NAME,
getName()
);
```
--
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]
