-
Notifications
You must be signed in to change notification settings - Fork 14
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
Distribution aggregation #22
Distribution aggregation #22
Conversation
…Strategy. Simplifying
|
||
@Override | ||
public void consume(BulletRecord data) { | ||
Number value = Specification.extractFieldAsNumber(field, data); |
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.
Is there a reason this function (extractFieldAsNumber()) is in Specification? Would it make sense in Utilities? Or maybe even BulletRecord or something?
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.
Yeah, it should be in BulletRecord but this was the best place to put it without making a Utilities in parsing. Since the field String using the '.' notation is a parsing thing, I don't want to put it in a top-level Utilities.
"OR specify a start, end and increment (start < end, increment > 0) to generate points"); | ||
public static final Error REQUIRES_POINTS_PROPER_RANGE = | ||
makeError(DistributionType.QUANTILE.getName() + " requires points in the proper range", | ||
"Please add or generate points: 0 <= point <= 1"); |
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.
Indentation?
SUPPORTED_DISTRIBUTION_TYPES.keySet().stream().collect(Collectors.joining(", "))); | ||
public static final Error REQUIRES_POINTS_ERROR = | ||
makeError("The DISTRIBUTION type requires at least one point", | ||
"Please add a list of numeric points, OR specify a number of equidistant points to generate" + |
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.
Maybe this error should mention the key of the list/points their query is lacking? Maybe something like "Please add a list of numeric points as 'numberOfPoints', OR specify a number of equidistant points to generate as 'points'.... etc?
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.
Will do the names of the keys.
if (Utilities.isEmpty(points)) { | ||
return null; | ||
} | ||
// Sort and get first maxPoints distinct values |
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.
Do we want to sort a list of points that a user provides? Is this necessary for the QuantileSketch constructor or something?
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.
Yes, needed by the QuantileSketch but not the constructor.
} | ||
|
||
/** | ||
* Validates whether the provided {@link Collection} of {@link GroupOperation} are valid. |
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.
is* valid?
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.
I was referring to operations conceptually not the Collection. Can change.
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.
Just minor stuff (5 suggestions). Overall looks great!!
} | ||
|
||
/** | ||
* Parses a {@link Set} of group operation from an Object that is expected to be a {@link List} of {@link Map}. |
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.
group operations* ?
} | ||
|
||
@Override | ||
protected Integer getSize() { |
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.
Should this do a collect() first?
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.
...Same with the below functions?
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.
No, they are protected or private not public and are meant to be called internally or by subclasses only.
private static double[] getPoints(double start, double end, int numberOfPoints) { | ||
double[] points = new double[numberOfPoints]; | ||
|
||
// We should have points < 1 but just in case... |
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.
points > 1 ?
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.
Yeah, meant to say We should not
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.
Also, since I do this check I should initialize the array after. It won't happen because the validations have already been done in Distribution but since I check here, I should do that too. Will make that change.
|
||
@Override | ||
protected Boolean isEstimationMode() { | ||
return merged.isEstimationMode(); |
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.
Does this need collect()? ...same with the functions below?
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.
Same reason as the comment above - internal or subclass use only.
No description provided.