FrankChen021 commented on code in PR #20070:
URL: https://github.com/apache/druid/pull/20070#discussion_r3813154077
##########
processing/src/main/java/org/apache/druid/query/aggregation/firstlast/first/DoubleFirstAggregatorFactory.java:
##########
@@ -59,32 +59,37 @@ public class DoubleFirstAggregatorFactory extends
AggregatorFactory
{
public static final ColumnType TYPE =
ColumnType.ofComplex(SerializablePairLongDoubleComplexMetricSerde.TYPE_NAME);
- private static final Aggregator NIL_AGGREGATOR = new DoubleFirstAggregator(
- NilColumnValueSelector.instance(),
- NilColumnValueSelector.instance(),
- false
- )
+ private static final Aggregator NIL_AGGREGATOR = new
NilDoubleFirstAggregator();
Review Comment:
[P2] Anonymous NIL implementations remain elsewhere
The conversion is incomplete: anonymous NIL implementations remain in
StringFirstAggregatorFactory, StringLastAggregatorFactory, and the numeric Any
factories. The same class-name serialization risk therefore remains for those
absent-column paths if runtime aggregators are serialized.
##########
processing/src/main/java/org/apache/druid/query/aggregation/firstlast/last/LongLastAggregatorFactory.java:
##########
@@ -60,31 +60,37 @@ public class LongLastAggregatorFactory extends
AggregatorFactory
{
public static final ColumnType TYPE =
ColumnType.ofComplex(SerializablePairLongLongComplexMetricSerde.TYPE_NAME);
- private static final Aggregator NIL_AGGREGATOR = new LongLastAggregator(
- NilColumnValueSelector.instance(),
- NilColumnValueSelector.instance(),
- false
- )
+ private static final Aggregator NIL_AGGREGATOR = new NilLongLastAggregator();
Review Comment:
[P2] No serialization regression test covers the fix
Existing serde tests only deserialize hard-coded factory JSON; they do not
serialize a factory or exercise NIL factorization. Add round-trip coverage for
the affected types and the missing-column branch.
##########
processing/src/main/java/org/apache/druid/query/aggregation/firstlast/last/LongLastAggregatorFactory.java:
##########
@@ -60,31 +60,37 @@ public class LongLastAggregatorFactory extends
AggregatorFactory
{
public static final ColumnType TYPE =
ColumnType.ofComplex(SerializablePairLongLongComplexMetricSerde.TYPE_NAME);
- private static final Aggregator NIL_AGGREGATOR = new LongLastAggregator(
- NilColumnValueSelector.instance(),
- NilColumnValueSelector.instance(),
- false
- )
+ private static final Aggregator NIL_AGGREGATOR = new NilLongLastAggregator();
+
+ private static class NilLongLastAggregator extends LongLastAggregator
Review Comment:
[P1] Named NIL class does not fix the Jackson type ID
NilLongLastAggregator extends LongLastAggregator, not AggregatorFactory. The
Jackson subtype registration applies to the factory, while the static NIL field
is not serialized. If the NIL object is serialized directly, it still has no
registered longLast type, so the change does not demonstrably fix the reported
serialization failure.
--
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]
