Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize CompressedBigDecimal compareTo() #13086

Merged
merged 1 commit into from
Sep 22, 2022
Merged

Optimize CompressedBigDecimal compareTo() #13086

merged 1 commit into from
Sep 22, 2022

Conversation

rash67
Copy link
Contributor

@rash67 rash67 commented Sep 14, 2022 •
edited

Optimize CompressedBigDecimal compareTo()

As titled, this optimizes the compareTo() function in
CompressedBigDecimal. It does so in the case that the scale 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
  • [ 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 is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@rash67 rash67 force-pushed the comp-big-decimal-work-comparator branch from ec29272 to 6333c05 Compare September 14, 2022 03:23
rash67
rash67 commented Sep 14, 2022
View reviewed changes
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Outdated
}
return this.toBigDecimal().compareTo(o.toBigDecimal());

int signum = signumInternal(result.length, result, (r, i) -> r[i]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rash67 rash67 force-pushed the comp-big-decimal-work-comparator branch 3 times, most recently from bc0f60a to f12dd1f Compare September 14, 2022 22:33
FrankChen021
FrankChen021 reviewed Sep 15, 2022
View reviewed changes
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Outdated
@@ -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)
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidental change? And I think the javadoc for this method is not helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FrankChen021
FrankChen021 reviewed Sep 15, 2022
View reviewed changes
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java
{
return compareTo(o, false);
}
public int compareTo(CompressedBigDecimal o, boolean expectOptimized)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rash67 rash67 force-pushed the comp-big-decimal-work-comparator branch from f12dd1f to 78bef43 Compare September 15, 2022 18:23
@rash67
Copy link
Contributor Author

rash67 commented Sep 15, 2022

I made an update so it can compare CBDs of different sizes and updated tests

@rash67 rash67 mentioned this pull request Sep 16, 2022
Merged
7 tasks
imply-cheddar
imply-cheddar approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@imply-cheddar imply-cheddar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions, but overall approved.

...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Outdated Show resolved Hide resolved
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Show resolved Hide resolved
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Outdated Show resolved Hide resolved
...sed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimal.java Outdated Show resolved Hide resolved
@rash67 rash67 force-pushed the comp-big-decimal-work-comparator branch 2 times, most recently from e3167b2 to a8e1619 Compare September 20, 2022 22:29
@rash67
Optimize CompressedBigDecimal compareTo()
ebc8090 
As titled, this optimizes the compareTo() function in
CompressedBigDecimal. It directly compares the int[] rather than
creating BigDecimal objects and using its compareTo.

It handles unequal sized CBDs, but does require
the scales to match.
@rash67 rash67 force-pushed the comp-big-decimal-work-comparator branch from a8e1619 to ebc8090 Compare September 21, 2022 22:40
@cheddar cheddar merged commit 044cab5 into apache:master Sep 22, 2022
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
This was referenced Dec 18, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheddar cheddar cheddar left review comments

@FrankChen021 FrankChen021 FrankChen021 left review comments

@imply-cheddar imply-cheddar imply-cheddar approved these changes

Assignees
No one assigned
Labels
Area - Extension
Projects
None yet
Milestone
25.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rash67 @cheddar @FrankChen021 @imply-cheddar @kfaraz