-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
input type validation for datasketches hll "build" aggregator factory #12131
Conversation
@@ -71,6 +74,7 @@ protected byte getCacheTypeId() | |||
public Aggregator factorize(final ColumnSelectorFactory columnSelectorFactory) | |||
{ | |||
final ColumnValueSelector<Object> selector = columnSelectorFactory.makeColumnValueSelector(getFieldName()); | |||
validateInputs(columnSelectorFactory.getColumnCapabilities(getFieldName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factorizeBuffer
and factorizeVector
should also validate their inputs
also please add tests for this stuff
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() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is used at both query and ingestion time, and shouldn't mention ingestion
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() | |
); | |
throw new ISE( | |
"Invalid input [%s] of type [%s] for [%s] aggregator [%s]", | |
getFieldName(), | |
capabilities.asTypeString(), | |
HllSketchModule.BUILD_TYPE_NAME, | |
getName() | |
); |
@@ -114,4 +118,19 @@ public int getMaxIntermediateSize() | |||
return HllSketch.getMaxUpdatableSerializationBytes(getLgK(), TgtHllType.valueOf(getTgtHllType())); | |||
} | |||
|
|||
private void validateInputs(@Nullable ColumnCapabilities capabilities) | |||
{ | |||
if (capabilities != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can use Types.is(capabilities, ValueType.COMPLEX)
to get rid of outer if statement since it null checks
Addressed the changes and added junit test case |
…apache#12131) * Ingestion will fail for HLLSketchBuild instead of creating with incorrect values * Addressing review comments for HLL< updated error message introduced test case (cherry picked from commit 08fea7a)
…apache#12131) * Ingestion will fail for HLLSketchBuild instead of creating with incorrect values * Addressing review comments for HLL< updated error message introduced test case (cherry picked from commit 08fea7a)
…rect values
Say we are ingesting the following dimensions for a complex data type, say hyperUnique
The ingestion will throw this error message
This PR has: