Copilot commented on code in PR #19390:
URL: https://github.com/apache/druid/pull/19390#discussion_r3159088966
##########
processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericAggregatorFactory.java:
##########
@@ -111,6 +116,27 @@ public final int getMaxIntermediateSizeWithNulls()
return getMaxIntermediateSize() + Byte.BYTES;
}
+ private boolean useNullableNumericAggregators(ColumnInspector
columnInspector)
+ {
+ if (forceNotNullable()) {
+ return false;
+ }
+
+ final String inputColumn = getInputColumn();
+ if (inputColumn == null) {
+ return true;
+ }
+
+ final ColumnCapabilities capabilities =
columnInspector.getColumnCapabilities(inputColumn);
+ return !(Types.isNumeric(capabilities) &&
capabilities.hasNulls().isFalse());
+ }
Review Comment:
The new optimization in `NullableNumericAggregatorFactory` changes wrapper
selection for Aggregator, BufferAggregator, and VectorAggregator paths, but the
added coverage in `DoubleSumAggregatorTest` only asserts behavior for
`factorizeBuffered`. Consider adding tests that exercise `factorize` and (if
feasible in tests) `factorizeVector` for both `hasNulls=false` and
unknown-nullability inputs, to ensure the wrapper-skipping logic is consistent
across all factorization methods.
--
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]
