LakshSingla commented on code in PR #14462:
URL: https://github.com/apache/druid/pull/14462#discussion_r1393669010
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector =
columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities =
columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector =
columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector =
columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector =
ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
Review Comment:
I am not sure if this is correct. If the type is numeric, why do we need an
explicit cast to `Double`. I could be wrong, but this seems kinda suspicious.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -164,7 +215,7 @@ void updateTimeWithNull(ByteBuffer buf, int position, long
time)
* Abstract function which needs to be overridden by subclasses to set the
* latest value in the buffer depending on the datatype
*/
- abstract void putValue(ByteBuffer buf, int position, int index);
+ abstract void putValue(ByteBuffer buf, int position, Number number);
Review Comment:
Let's revert this change if we can. It will lead to some duplication,
however, this will lead to the boxing of the primitives, which is what the
column selectors aim to reduce (if they can). Therefore there are separate
methods like `isNull` + `getLong` which are preferred instead of `getObject`
because the former doesn't auto-box the primitive variable to an object.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -132,14 +183,14 @@ public void aggregate(
*
* @param buf byte buffer storing the byte array representation of
the aggregate
* @param position offset within the byte buffer at which the current
aggregate value is stored
- * @param time the time to be updated in the buffer as the last time
- * @param index the index of the vectorized vector which is the last
value
+ * @param time the time to be updated in the buffer as the first time
+ * @param number number which is the first value
*/
- void updateTimeWithValue(ByteBuffer buf, int position, long time, int index)
+ void updateTimeWithValue(ByteBuffer buf, int position, long time, Number
number)
Review Comment:
It might not be as clean as the current code, however, there shouldn't be a
lot of duplication.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/NumericFirstVectorAggregator.java:
##########
@@ -132,14 +183,14 @@ public void aggregate(
*
* @param buf byte buffer storing the byte array representation of
the aggregate
* @param position offset within the byte buffer at which the current
aggregate value is stored
- * @param time the time to be updated in the buffer as the last time
- * @param index the index of the vectorized vector which is the last
value
+ * @param time the time to be updated in the buffer as the first time
+ * @param number number which is the first value
*/
- void updateTimeWithValue(ByteBuffer buf, int position, long time, int index)
+ void updateTimeWithValue(ByteBuffer buf, int position, long time, Number
number)
Review Comment:
Autoboxing alert, as mentioned in the other comment. Let's see if we can get
away without this change.
##########
processing/src/main/java/org/apache/druid/query/aggregation/first/DoubleFirstAggregatorFactory.java:
##########
@@ -142,12 +160,24 @@ public VectorAggregator factorizeVector(
VectorColumnSelectorFactory columnSelectorFactory
)
{
+ VectorValueSelector timeSelector =
columnSelectorFactory.makeValueSelector(timeColumn);
ColumnCapabilities capabilities =
columnSelectorFactory.getColumnCapabilities(fieldName);
+
if (Types.isNumeric(capabilities)) {
VectorValueSelector valueSelector =
columnSelectorFactory.makeValueSelector(fieldName);
- VectorValueSelector timeSelector =
columnSelectorFactory.makeValueSelector(
- timeColumn);
- return new DoubleFirstVectorAggregator(timeSelector, valueSelector);
+ VectorObjectSelector objectSelector =
ExpressionVectorSelectors.castValueSelectorToObject(
+ columnSelectorFactory.getReadableVectorInspector(),
+ fieldName,
+ valueSelector,
+ capabilities.toColumnType(),
+ ColumnType.DOUBLE
+ );
+ return new DoubleFirstVectorAggregator(timeSelector, objectSelector);
+ }
+
+ VectorObjectSelector vSelector =
columnSelectorFactory.makeObjectSelector(fieldName);
+ if (capabilities != null) {
+ return new DoubleFirstVectorAggregator(timeSelector, vSelector);
Review Comment:
Why is this check required? I don't think we are using the capabilities
inside the aggregator
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
