You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/01/07 21:11:27 UTC
[GitHub] [druid] clintropolis commented on a change in pull request #12131: Ingestion will fail for HLLSketchBuild instead of creating with incor…
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: 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