-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
GH-37701: [Java] Add default comparators for more types #37748
GH-37701: [Java] Add default comparators for more types #37748
Conversation
Add default type comparators for: - BitVector - DateDayVector - DateMilliVector - Decimal256Vector - DecimalVector - DurationVector - IntervalDayVector - TimeMicroVector - TimeMilliVector - TimeNanoVector - TimeSecVector - TimeStampVector IntervalMonthDayNanoVector is not supported due to its public type PeriodDuration not being Comparable.
|
||
@Override | ||
public int compareNotNull(int index1, int index2) { | ||
BigDecimal value1 = vector1.getObjectNotNull(index1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like VectorValueComparator ensures the slot is not null before calling this, so is adding the new getObjectNotNull method necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And if it is necessary, it might be preferable to use the regular getObject and replace any nulls with a sentinel value, since otherwise there is no guarantee on the value that is actually in the null slot - but it seems that shouldn't happen in the first place.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent with the getObjectNotNull() method is to provide a fast-path getter that skips checking the null slot (since VectorValueComparator has done the null check already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, makes sense.
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 3db6205. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…#37748) ### Rationale for this change Add default comparators for more vector types to make algorithms easier to use and provide more consistency for Java compared to other languages. ### What changes are included in this PR? Add default type comparators for: - BitVector - DateDayVector - DateMilliVector - Decimal256Vector - DecimalVector - DurationVector - IntervalDayVector - TimeMicroVector - TimeMilliVector - TimeNanoVector - TimeSecVector - TimeStampVector IntervalMonthDayNanoVector is not supported due to its public type PeriodDuration not being Comparable. BitVector's getWidth() method does not return valid data by design since its length is smaller than 1 byte. Using a BitVector with a fixed-width type's algorithm will throw an IllegalArgumentException. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#37701 Authored-by: James Duong <duong.james@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…#37748) ### Rationale for this change Add default comparators for more vector types to make algorithms easier to use and provide more consistency for Java compared to other languages. ### What changes are included in this PR? Add default type comparators for: - BitVector - DateDayVector - DateMilliVector - Decimal256Vector - DecimalVector - DurationVector - IntervalDayVector - TimeMicroVector - TimeMilliVector - TimeNanoVector - TimeSecVector - TimeStampVector IntervalMonthDayNanoVector is not supported due to its public type PeriodDuration not being Comparable. BitVector's getWidth() method does not return valid data by design since its length is smaller than 1 byte. Using a BitVector with a fixed-width type's algorithm will throw an IllegalArgumentException. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#37701 Authored-by: James Duong <duong.james@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
Add default comparators for more vector types to make algorithms easier to use and provide more consistency for Java compared to other languages.
What changes are included in this PR?
Add default type comparators for:
IntervalMonthDayNanoVector is not supported due to its public type PeriodDuration not being Comparable.
BitVector's getWidth() method does not return valid data by design since its length is smaller than 1 byte. Using a BitVector with a fixed-width type's algorithm will throw an IllegalArgumentException.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.