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/14 03:19:17 UTC
[GitHub] [druid] rash67 opened a new pull request, #13086: Optimize CompressedBigDecimal compareTo()
rash67 opened a new pull request, #13086:
URL: https://github.com/apache/druid/pull/13086
## Optimize CompressedBigDecimal compareTo()
As titled, this optimizes the compareTo() function in
CompressedBigDecimal. It does so in the case that the array size is
equal.
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 pull request #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
rash67 commented on PR #13086:
URL: https://github.com/apache/druid/pull/13086#issuecomment-1248449370
I made an update so it can compare CBDs of different sizes and updated tests
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r975819012
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -271,16 +273,82 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
}
}
- /* (non-Javadoc)
- * @see java.lang.Comparable#compareTo(java.lang.Object)
- */
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale()) {
+ return directCompareCompressedBigDecimal(this, o, Math.max(getArraySize(), o.getArraySize()));
+ } else {
+ if (expectOptimized) {
+ throw new IAE("expected optimized path");
+ }
+
+ return this.toBigDecimal().compareTo(o.toBigDecimal());
+ }
+ }
+
+ /**
+ * performs a subtraction of lhs - rhs to compare elements
+ *
+ * @param lhs
+ * @param rhs
+ * @param length - length of both lhs and rhs
+ * @return
+ */
+ private static int directCompareCompressedBigDecimal(CompressedBigDecimal lhs, CompressedBigDecimal rhs, int length)
+ {
+ int[] result = new int[length];
+ int borrow = 0;
+ // for each argument, if it's negative, our extension will be Integer.MIN_VALUE (all 1s). else, all 0s
+ long lhsExtension = lhs.getArrayEntry(lhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+ long rhsExtension = rhs.getArrayEntry(rhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+
+ for (int i = 0; i < length; i++) {
+ // "dynamically" extend lhs/rhs if it's shorter than the other using extensions computed above
+ long leftElement = i < lhs.getArraySize() ? (INT_MASK & lhs.getArrayEntry(i)) : lhsExtension;
+ long rightElement = i < rhs.getArraySize() ? (INT_MASK & rhs.getArrayEntry(i)) : rhsExtension;
+ long resultElement = leftElement - rightElement - borrow;
+
+ borrow = 0;
+
+ if (resultElement < 0) {
+ borrow = 1;
+ resultElement += 1L << 32;
+ }
+
+ result[i] = (int) resultElement;
+ }
+
+ int signum = signumInternalArray(result.length, result);
+
+ return signum;
+ }
+
+ /**
+ * specialized signumInternal to avoid meagmorphic callsite in signumInternal's array accessor lambda
+ */
+ private static int signumInternalArray(int size, int[] valueArray)
+ {
+ if (valueArray[size - 1] < 0) {
+ return -1;
+ } else {
+ int agg = 0;
+ for (int i = 0; i < size; ++i) {
+ agg |= valueArray[i];
+ }
+ if (agg == 0) {
Review Comment:
maybe an over-optimization, but i went ahead and did it as it's simple enough.
it also obviates the "specialized" method signumInternalArray() so i can remove it.
win-win
--
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] FrankChen021 commented on a diff in pull request #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r971532276
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
Review Comment:
accidental change? And I think the javadoc for this method is not helpful.
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r972230866
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
Review Comment:
it's called in the unit test. It was a suggestion by @cheddar as a way to detect a regression in the optimized path of compareTo() in unit tests.
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r974812934
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -271,16 +273,82 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
}
}
- /* (non-Javadoc)
- * @see java.lang.Comparable#compareTo(java.lang.Object)
- */
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale()) {
+ return directCompareCompressedBigDecimal(this, o, Math.max(getArraySize(), o.getArraySize()));
+ } else {
+ if (expectOptimized) {
+ throw new IAE("expected optimized path");
+ }
+
+ return this.toBigDecimal().compareTo(o.toBigDecimal());
+ }
+ }
+
+ /**
+ * performs a subtraction of lhs - rhs to compare elements
+ *
+ * @param lhs
+ * @param rhs
+ * @param length - length of both lhs and rhs
+ * @return
+ */
+ private static int directCompareCompressedBigDecimal(CompressedBigDecimal lhs, CompressedBigDecimal rhs, int length)
+ {
+ int[] result = new int[length];
+ int borrow = 0;
+ // for each argument, if it's negative, our extension will be Integer.MIN_VALUE (all 1s). else, all 0s
+ long lhsExtension = lhs.getArrayEntry(lhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+ long rhsExtension = rhs.getArrayEntry(rhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+
+ for (int i = 0; i < length; i++) {
+ // "dynamically" extend lhs/rhs if it's shorter than the other using extensions computed above
+ long leftElement = i < lhs.getArraySize() ? (INT_MASK & lhs.getArrayEntry(i)) : lhsExtension;
+ long rightElement = i < rhs.getArraySize() ? (INT_MASK & rhs.getArrayEntry(i)) : rhsExtension;
+ long resultElement = leftElement - rightElement - borrow;
Review Comment:
Is there room for overflow from this? I'm asking without thinking about it, so "no" is a totally possible answer.
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -271,16 +273,82 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
}
}
- /* (non-Javadoc)
- * @see java.lang.Comparable#compareTo(java.lang.Object)
- */
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale()) {
+ return directCompareCompressedBigDecimal(this, o, Math.max(getArraySize(), o.getArraySize()));
+ } else {
+ if (expectOptimized) {
+ throw new IAE("expected optimized path");
+ }
+
+ return this.toBigDecimal().compareTo(o.toBigDecimal());
+ }
+ }
+
+ /**
+ * performs a subtraction of lhs - rhs to compare elements
+ *
+ * @param lhs
+ * @param rhs
+ * @param length - length of both lhs and rhs
+ * @return
+ */
+ private static int directCompareCompressedBigDecimal(CompressedBigDecimal lhs, CompressedBigDecimal rhs, int length)
+ {
+ int[] result = new int[length];
+ int borrow = 0;
+ // for each argument, if it's negative, our extension will be Integer.MIN_VALUE (all 1s). else, all 0s
+ long lhsExtension = lhs.getArrayEntry(lhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+ long rhsExtension = rhs.getArrayEntry(rhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+
+ for (int i = 0; i < length; i++) {
+ // "dynamically" extend lhs/rhs if it's shorter than the other using extensions computed above
+ long leftElement = i < lhs.getArraySize() ? (INT_MASK & lhs.getArrayEntry(i)) : lhsExtension;
+ long rightElement = i < rhs.getArraySize() ? (INT_MASK & rhs.getArrayEntry(i)) : rhsExtension;
+ long resultElement = leftElement - rightElement - borrow;
+
+ borrow = 0;
+
+ if (resultElement < 0) {
+ borrow = 1;
+ resultElement += 1L << 32;
+ }
+
+ result[i] = (int) resultElement;
+ }
+
+ int signum = signumInternalArray(result.length, result);
+
+ return signum;
+ }
+
+ /**
+ * specialized signumInternal to avoid meagmorphic callsite in signumInternal's array accessor lambda
+ */
+ private static int signumInternalArray(int size, int[] valueArray)
+ {
+ if (valueArray[size - 1] < 0) {
+ return -1;
+ } else {
+ int agg = 0;
+ for (int i = 0; i < size; ++i) {
+ agg |= valueArray[i];
+ }
+ if (agg == 0) {
Review Comment:
Totally random, probably over-optimization (if even an optimization), but it strikes me that you could keep track of whether the `result` values are always 0 while doing the subtraction and then you would already know if they are already 0 without needing to pass over the valueArray again.
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -271,16 +273,82 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
}
}
- /* (non-Javadoc)
- * @see java.lang.Comparable#compareTo(java.lang.Object)
- */
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale()) {
+ return directCompareCompressedBigDecimal(this, o, Math.max(getArraySize(), o.getArraySize()));
+ } else {
+ if (expectOptimized) {
+ throw new IAE("expected optimized path");
+ }
+
+ return this.toBigDecimal().compareTo(o.toBigDecimal());
+ }
+ }
+
+ /**
+ * performs a subtraction of lhs - rhs to compare elements
+ *
+ * @param lhs
+ * @param rhs
+ * @param length - length of both lhs and rhs
+ * @return
+ */
+ private static int directCompareCompressedBigDecimal(CompressedBigDecimal lhs, CompressedBigDecimal rhs, int length)
+ {
+ int[] result = new int[length];
+ int borrow = 0;
+ // for each argument, if it's negative, our extension will be Integer.MIN_VALUE (all 1s). else, all 0s
+ long lhsExtension = lhs.getArrayEntry(lhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+ long rhsExtension = rhs.getArrayEntry(rhs.getArraySize() - 1) < 0 ? INT_MASK : 0;
+
+ for (int i = 0; i < length; i++) {
+ // "dynamically" extend lhs/rhs if it's shorter than the other using extensions computed above
+ long leftElement = i < lhs.getArraySize() ? (INT_MASK & lhs.getArrayEntry(i)) : lhsExtension;
+ long rightElement = i < rhs.getArraySize() ? (INT_MASK & rhs.getArrayEntry(i)) : rhsExtension;
+ long resultElement = leftElement - rightElement - borrow;
+
+ borrow = 0;
+
+ if (resultElement < 0) {
+ borrow = 1;
+ resultElement += 1L << 32;
+ }
+
+ result[i] = (int) resultElement;
+ }
+
+ int signum = signumInternalArray(result.length, result);
+
+ return signum;
+ }
+
+ /**
+ * specialized signumInternal to avoid meagmorphic callsite in signumInternal's array accessor lambda
Review Comment:
typo: megamorphic
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -271,16 +273,82 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
}
}
- /* (non-Javadoc)
- * @see java.lang.Comparable#compareTo(java.lang.Object)
- */
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale()) {
+ return directCompareCompressedBigDecimal(this, o, Math.max(getArraySize(), o.getArraySize()));
Review Comment:
Is there a reason to do this `max` here instead of as the first thing after entering into the `directCompareCompressedBigDecimal` call?
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r972229862
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
Review Comment:
yea, i'm not sure what happened here. there are a lot of these javadoc comments from the original PR that was merged :/
i'll remove it
--
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 commented on a diff in pull request #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
cheddar commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r977146336
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
Review Comment:
Heh, he's not doing it that way because I hate the `VisibleForTesting` annotation and adjusting the visibility only creates friction for the future without actually protecting anything. Those things combine to just pretend like the method is not part of the actual contract of the object when it is.
In this case, there is basically a "strict" version that expects that objects have already been aligned so that we can use a more optimal code path. But there's a more loose version as well that just "does the right thing". We have the tests that exercise this via queries only use the "strict" version because the normal query pipeline should be aligning things so that the more optimal code path is followed, but in production the non-strict version is actually used just in case. I.e. the tests are there to catch against regressions when running through queries, but there are other tests that also leverage the whole thing. In all, the "contract" of the object requires us to have both, otherwise we cannot test it. There also might be a case in production where we want to blow up if there is a mis-alignment, in that case, this method is a great way to achieve that.
--
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] FrankChen021 commented on a diff in pull request #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r977126650
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
Review Comment:
if it's called by the UT only, usually we declare its visibility as package level, and use `VisibileForTesting` annotation to make it more intuitive.
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
cheddar merged PR #13086:
URL: https://github.com/apache/druid/pull/13086
--
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] FrankChen021 commented on a diff in pull request #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r971534043
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -274,13 +276,74 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
/* (non-Javadoc)
* @see java.lang.Comparable#compareTo(java.lang.Object)
*/
+
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
Review Comment:
I notice that this method is called by `compareTo` method at line 283, and a `false` is passed to this method as 2nd parameter. I don't find any other places that call this method with a `true` value.
--
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 #13086: Optimize CompressedBigDecimal compareTo()
Posted by GitBox <gi...@apache.org>.
rash67 commented on code in PR #13086:
URL: https://github.com/apache/druid/pull/13086#discussion_r970271401
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java:
##########
@@ -276,13 +278,48 @@ protected static <S> int signumInternal(int size, S rhs, ToIntBiFunction<S, Inte
*/
@Override
public int compareTo(CompressedBigDecimal o)
+ {
+ return compareTo(o, false);
+ }
+
+ public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
{
if (super.equals(o)) {
return 0;
+ } else if (getScale() == o.getScale() && getArraySize() == o.getArraySize()) {
+ return compareCompressedBigDecimal(this, o, getArraySize());
+ } else {
+ if (expectOptimized) {
+ throw new RE("expected optimized path");
+ }
+
+ return this.toBigDecimal().compareTo(o.toBigDecimal());
+ }
+ }
+
+ public static int compareCompressedBigDecimal(CompressedBigDecimal lhs, CompressedBigDecimal rhs, int length)
+ {
+ int[] result = new int[length];
+ int borrow = 0;
+
+ for (int i = 0; i < length; i++) {
+ long x = (INT_MASK & lhs.getArrayEntry(i)) - (INT_MASK & rhs.getArrayEntry(i)) - borrow;
+ borrow = 0;
+
+ if (x < 0) {
+ borrow = 1;
+ x += 1L << 32;
+ }
+
+ result[i] = (int) x;
}
- return this.toBigDecimal().compareTo(o.toBigDecimal());
+
+ int signum = signumInternal(result.length, result, (r, i) -> r[i]);
Review Comment:
not sure if i should create a concrete class or concrete impl of signumInternal for this case to avoid megamorphic callsite for the lambda passed in?
--
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