add substituteCombiningFactory implementations for datasketches aggs#17314
add substituteCombiningFactory implementations for datasketches aggs#17314kfaraz merged 3 commits intoapache:masterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public final CloserRule closer = new CloserRule(false); | ||
|
|
||
| public DatasketchesProjectionTest( | ||
| String name, |
Check notice
Code scanning / CodeQL
Useless parameter
| that.fieldName | ||
| )) { | ||
| if (lgK <= that.lgK && | ||
| tgtHllType.ordinal() <= that.tgtHllType.ordinal() && |
There was a problem hiding this comment.
I don't think the TgtHllType needs to be part of the check here, since according to the datasketches docs:
These three target types are isomorphic representations of the same underlying HLL algorithm. Thus, given the same value of lgConfigK and the same input, all three HLL target types will produce identical estimates and have identical error distributions.
& I think that the aggregator is able to merge two different HLL types together.
| } | ||
|
|
||
| @Override | ||
| public AggregatorFactory substituteCombiningFactory(AggregatorFactory preAggregated) |
There was a problem hiding this comment.
does SketchMergeAggregatorFactory need to override this with an impl that checks isInputThetaSketch?
There was a problem hiding this comment.
hmm, i suppose it wouldn't hurt, though i can't imagine the field would be the same name and isInputThetaSketch be true in query agg but not in projection agg, or the other way, can add just in case since i need to make some other changes
|
|
||
| ArrayOfDoublesSketchAggregatorFactory that = (ArrayOfDoublesSketchAggregatorFactory) preAggregated; | ||
| if (nominalEntries <= that.nominalEntries && | ||
| numberOfValues <= that.numberOfValues && |
There was a problem hiding this comment.
I think numberOfValues needs to match. There's some code in ArrayOfDoublesUnion like:
if (gadget_.getNumValues() != tupleSketch.getNumValues()) {
throw new SketchesArgumentException("Incompatible sketches: number of values mismatch "
+ gadget_.getNumValues() + " and " + tupleSketch.getNumValues());
}
There was a problem hiding this comment.
ah yeah, good catch
…pache#17314) Follow up to apache#17214, adds implementations for substituteCombiningFactory so that more datasketches aggs can match projections, along with some projections tests for datasketches.
4 participants
Follow up to #17214, adds implementations for
substituteCombiningFactoryso that more datasketches aggs can match projections, along with some projections tests for datasketches.