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/08 02:27:16 UTC
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13048: Compressed Big Decimal Cleanup and Extension
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