-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Speed up String first/last aggregators when folding isn't needed. #9181
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
Changes from all commits
ecc4310
92f2218
c56d895
f0d0075
de0697c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ | |
| import org.apache.druid.segment.BaseLongColumnValueSelector; | ||
| import org.apache.druid.segment.BaseObjectColumnValueSelector; | ||
| import org.apache.druid.segment.DimensionHandlerUtils; | ||
| import org.apache.druid.segment.NilColumnValueSelector; | ||
| import org.apache.druid.segment.column.ColumnCapabilities; | ||
| import org.apache.druid.segment.column.ValueType; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import java.nio.ByteBuffer; | ||
|
|
@@ -32,10 +35,34 @@ public class StringFirstLastUtils | |
| { | ||
| private static final int NULL_VALUE = -1; | ||
|
|
||
| /** | ||
| * Returns whether a given value selector *might* contain SerializablePairLongString objects. | ||
| */ | ||
| public static boolean selectorNeedsFoldCheck( | ||
| final BaseObjectColumnValueSelector<?> valueSelector, | ||
| @Nullable final ColumnCapabilities valueSelectorCapabilities | ||
| ) | ||
| { | ||
| if (valueSelectorCapabilities != null && valueSelectorCapabilities.getType() != ValueType.COMPLEX) { | ||
| // Known, non-complex type. | ||
| return false; | ||
| } | ||
|
|
||
| if (valueSelector instanceof NilColumnValueSelector) { | ||
| // Nil column, definitely no SerializablePairLongStrings. | ||
| return false; | ||
| } | ||
|
|
||
| // Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass). | ||
| final Class<?> clazz = valueSelector.classOfObject(); | ||
| return clazz.isAssignableFrom(SerializablePairLongString.class) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it looks to me like this method only returns true if a selector definitely is a Alternatively, should
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies, I was wrong, I think this line + comment just isn't super intuitive on first glance that it is a catch-all that any ‘unknown’ selector types will also be true (since clazz will be Object.class in that case). Maybe expanding on the comment would make it require less thought?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to: // Check if the selector class could possibly be a SerializablePairLongString (either a superclass or subclass). |
||
| || SerializablePairLongString.class.isAssignableFrom(clazz); | ||
| } | ||
|
|
||
| @Nullable | ||
| public static SerializablePairLongString readPairFromSelectors( | ||
| final BaseLongColumnValueSelector timeSelector, | ||
| final BaseObjectColumnValueSelector valueSelector | ||
| final BaseObjectColumnValueSelector<?> valueSelector | ||
| ) | ||
| { | ||
| final long time; | ||
|
|
||
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.
🙂