Repository navigation
Compressed Big Decimal Cleanup and Extension - #13048
Conversation
| if (selectedObject != null) { | ||
| if (selectedObject.getScale() != sum.getScale()) { | ||
| selectedObject = Utils.scaleUp(selectedObject); | ||
| selectedObject = Utils.scaleUp(selectedObject, sum.getScale()); |
There was a problem hiding this comment.
Is there a reason not to push the "scale up" part into the Utils.objToCompressedBigDecimal() call itself?
There was a problem hiding this comment.
i don't think so. we should have this function throw an exception if the scales don't match. it doesn't even document that the scales must match. you have to follow into internalAdd and see that the scale isn't examined
There was a problem hiding this comment.
oh and also Utils.objToCompressedBigDecimal() is called in the extractor which doesn't presently have the scale. unless ComplexMetricSerde can have a constructor with params?
There was a problem hiding this comment.
and also^2, CBD.accumulate() already does this, but it seems to use Utils.accumulate() to avoid object creation...
(which interestingly makes the generified cbd.accumulate() method with various lambdas kind of pointless...it will be monomorphic in practice. I think the method it uses maybe is only used in tests(?)
basically CBD.accumulate() is only ever called on ArrayCompressedBigDecimal
There was a problem hiding this comment.
it might be better to just have 2 impls of CompressedBigDecimal.accumulate() and have the buffer version use Utils.accumulate....
it would be easier to read code (though i'm a little scaled to change CompressedBigDecimal.internalAdd and break it....test coverage seems adequate
| CompressedBigDecimal<?> addend = selector.getObject(); | ||
| CompressedBigDecimal addend = Utils.objToCompressedBigDecimal(selector.getObject()); | ||
| if (addend != null) { | ||
| Utils.accumulate(buf, position, size, scale, addend); |
There was a problem hiding this comment.
Just double checking, this Utils.accumulate() will scale the scale right?
There was a problem hiding this comment.
yes, i moved it there because
- we have access to scale to pass in
- accumulate will always be functionally correct
the only reason to move it elsewhere is if we somehow were going to use a CBD more than once in a new scale to avoid repeatedly changing the scale. that doesn't seem the case in druid
c728946 to
ad10f41
Compare
1. remove unnecessary generic type from CompressedBigDecimal 2. support Number input types 3. support aggregator reading supported input types direclty (uningested data) 4. fix scaling bug in buffer aggregator
ad10f41 to
c312e72
Compare
1. remove unnecessary generic type from CompressedBigDecimal 2. support Number input types 3. support aggregator reading supported input types directly (uningested data) 4. fix scaling bug in buffer aggregator
5 participants
This commit:
This PR has: