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/09/07 21:42:02 UTC
[GitHub] [druid] rash67 opened a new pull request, #13048: Compressed Big Decimal Cleanup and Extension
rash67 opened a new pull request, #13048:
URL: https://github.com/apache/druid/pull/13048
This commit:
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
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [X ] been self-reviewed.
- [ X] added documentation for new or modified features or behaviors.
- [ X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ X] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967371283
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalAggregator.java:
##########
@@ -54,10 +54,10 @@ public CompressedBigDecimalAggregator(
@Override
public void aggregate()
{
- CompressedBigDecimal<?> selectedObject = selector.getObject();
+ CompressedBigDecimal selectedObject = Utils.objToCompressedBigDecimal(selector.getObject());
if (selectedObject != null) {
if (selectedObject.getScale() != sum.getScale()) {
- selectedObject = Utils.scaleUp(selectedObject);
+ selectedObject = Utils.scaleUp(selectedObject, sum.getScale());
Review 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`
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967367972
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalBufferAggregator.java:
##########
@@ -71,10 +71,11 @@ public void init(ByteBuffer buf, int position)
@Override
public void aggregate(ByteBuffer buf, int position)
{
- CompressedBigDecimal<?> addend = selector.getObject();
+ CompressedBigDecimal addend = Utils.objToCompressedBigDecimal(selector.getObject());
if (addend != null) {
Utils.accumulate(buf, position, size, scale, addend);
Review Comment:
yes, i moved it there because
1. we have access to scale to pass in
2. 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
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r965443333
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -266,15 +262,27 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
@Override
- public int compareTo(CompressedBigDecimal<T> o)
+ public int compareTo(CompressedBigDecimal o)
{
- if (this.equals(o)) {
+ if (super.equals(o)) {
return 0;
}
return this.toBigDecimal().compareTo(o.toBigDecimal());
}
+ @Override
+ public int hashCode()
+ {
+ return super.hashCode();
Review Comment:
I'm a little suspect of these `super` calls in hashCode and equals as well.
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -266,15 +262,27 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
@Override
- public int compareTo(CompressedBigDecimal<T> o)
+ public int compareTo(CompressedBigDecimal o)
{
- if (this.equals(o)) {
+ if (super.equals(o)) {
Review Comment:
Why the switch to super? Will `Number`'s `.equals` do good things?
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalAggregator.java:
##########
@@ -54,10 +54,10 @@ public CompressedBigDecimalAggregator(
@Override
public void aggregate()
{
- CompressedBigDecimal<?> selectedObject = selector.getObject();
+ CompressedBigDecimal selectedObject = Utils.objToCompressedBigDecimal(selector.getObject());
if (selectedObject != null) {
if (selectedObject.getScale() != sum.getScale()) {
- selectedObject = Utils.scaleUp(selectedObject);
+ selectedObject = Utils.scaleUp(selectedObject, sum.getScale());
Review Comment:
Is there a reason not to push the "scale up" part into the `Utils.objToCompressedBigDecimal()` call itself?
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalBufferAggregator.java:
##########
@@ -71,10 +71,11 @@ public void init(ByteBuffer buf, int position)
@Override
public void aggregate(ByteBuffer buf, int position)
{
- CompressedBigDecimal<?> addend = selector.getObject();
+ CompressedBigDecimal addend = Utils.objToCompressedBigDecimal(selector.getObject());
if (addend != null) {
Utils.accumulate(buf, position, size, scale, addend);
Review Comment:
Just double checking, this `Utils.accumulate()` will scale the scale right?
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalBufferAggregator.java:
##########
@@ -71,10 +71,11 @@ public void init(ByteBuffer buf, int position)
@Override
public void aggregate(ByteBuffer buf, int position)
{
- CompressedBigDecimal<?> addend = selector.getObject();
+ CompressedBigDecimal addend = Utils.objToCompressedBigDecimal(selector.getObject());
if (addend != null) {
Utils.accumulate(buf, position, size, scale, addend);
}
+ int x = 3;
Review Comment:
???
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967363966
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalAggregator.java:
##########
@@ -54,10 +54,10 @@ public CompressedBigDecimalAggregator(
@Override
public void aggregate()
{
- CompressedBigDecimal<?> selectedObject = selector.getObject();
+ CompressedBigDecimal selectedObject = Utils.objToCompressedBigDecimal(selector.getObject());
if (selectedObject != null) {
if (selectedObject.getScale() != sum.getScale()) {
- selectedObject = Utils.scaleUp(selectedObject);
+ selectedObject = Utils.scaleUp(selectedObject, sum.getScale());
Review 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
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967362309
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalBufferAggregator.java:
##########
@@ -71,10 +71,11 @@ public void init(ByteBuffer buf, int position)
@Override
public void aggregate(ByteBuffer buf, int position)
{
- CompressedBigDecimal<?> addend = selector.getObject();
+ CompressedBigDecimal addend = Utils.objToCompressedBigDecimal(selector.getObject());
if (addend != null) {
Utils.accumulate(buf, position, size, scale, addend);
}
+ int x = 3;
Review Comment:
debugging line (to enable breakpoint here
will remove
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967355538
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -266,15 +262,27 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
@Override
- public int compareTo(CompressedBigDecimal<T> o)
+ public int compareTo(CompressedBigDecimal o)
{
- if (this.equals(o)) {
+ if (super.equals(o)) {
Review Comment:
actually i need to leave it in. the update does change equals mostly because this overrode compareTo() without equals so I did a quick simple override of equals/hashcode to delegate to toBigDecimal.
it won't perf well but it keeps it functionally correct
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967372974
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalAggregator.java:
##########
@@ -54,10 +54,10 @@ public CompressedBigDecimalAggregator(
@Override
public void aggregate()
{
- CompressedBigDecimal<?> selectedObject = selector.getObject();
+ CompressedBigDecimal selectedObject = Utils.objToCompressedBigDecimal(selector.getObject());
if (selectedObject != null) {
if (selectedObject.getScale() != sum.getScale()) {
- selectedObject = Utils.scaleUp(selectedObject);
+ selectedObject = Utils.scaleUp(selectedObject, sum.getScale());
Review 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
--
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
[GitHub] [druid] cheddar merged pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
cheddar merged PR #13048:
URL: https://github.com/apache/druid/pull/13048
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967366766
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalAggregator.java:
##########
@@ -54,10 +54,10 @@ public CompressedBigDecimalAggregator(
@Override
public void aggregate()
{
- CompressedBigDecimal<?> selectedObject = selector.getObject();
+ CompressedBigDecimal selectedObject = Utils.objToCompressedBigDecimal(selector.getObject());
if (selectedObject != null) {
if (selectedObject.getScale() != sum.getScale()) {
- selectedObject = Utils.scaleUp(selectedObject);
+ selectedObject = Utils.scaleUp(selectedObject, sum.getScale());
Review 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?
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967353301
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -266,15 +262,27 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
@Override
- public int compareTo(CompressedBigDecimal<T> o)
+ public int compareTo(CompressedBigDecimal o)
{
- if (this.equals(o)) {
+ if (super.equals(o)) {
Review Comment:
oops, it was a hangover from when i was changing this.equals() and it wanted to do identity check (and java won't let you use == for Number identity check due to auto-unboxing ambiguity and CBD extends Number...
will revert
--
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
[GitHub] [druid] rash67 commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13048:
URL: https://github.com/apache/druid/pull/13048#discussion_r967355878
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -266,15 +262,27 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
@Override
- public int compareTo(CompressedBigDecimal<T> o)
+ public int compareTo(CompressedBigDecimal o)
{
- if (this.equals(o)) {
+ if (super.equals(o)) {
return 0;
}
return this.toBigDecimal().compareTo(o.toBigDecimal());
}
+ @Override
+ public int hashCode()
+ {
+ return super.hashCode();
Review Comment:
yea see above, i accidentally reverted a change to these
--
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