FrankChen021 commented on code in PR #20070:
URL: https://github.com/apache/druid/pull/20070#discussion_r4082438616
##########
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:
[P1] Named NIL class does not fix the Jackson type ID
**Finding:** The new NilLongLastAggregator is still an Aggregator, while
Jackson's subtype registration is on AggregatorFactory; the only references to
this static instance are the factorize/factorizeBuffered return paths, and the
factory serializer exposes only its name, fieldName, and timeColumn properties.
Consequently, this rename cannot change the AggregatorFactory type id or
resolve the reported LongLastAggregatorFactory$1 failure; if the runtime
aggregator is serialized through another path, the new private class is still
not registered.
**Suggestion:** Trace the actual object being serialized and fix or register
that Jackson-facing type; otherwise retitle or remove this no-op production
change and keep only a regression test for the real failure.
##########
processing/src/test/java/org/apache/druid/query/aggregation/firstlast/last/LongLastAggregationTest.java:
##########
@@ -235,6 +236,65 @@ public void testSerde() throws Exception
Assertions.assertArrayEquals(longLastAggFactory.getCacheKey(),
deserialized.getCacheKey());
}
+ /**
+ * Round-trips the factory through Jackson instead of only reading a
hard-coded JSON string.
+ * The pre-existing {@link #testSerde()} test never serializes, so it cannot
observe the type id
+ * Jackson actually writes out. A regression where an anonymous class
reached the jackson-facing
+ * surface would produce a type id such as {@code
LongLastAggregatorFactory$1}, which the old
+ * test would not catch because it never produces the JSON.
+ */
+ @Test
+ public void testSerdeRoundTrip() throws Exception
+ {
+ final DefaultObjectMapper mapper = new DefaultObjectMapper();
+ final String serialized = mapper.writeValueAsString(longLastAggFactory);
Review Comment:
[P2] Round-trip tests do not exercise the changed NIL classes
**Finding:** Both added assertions serialize longLastAggFactory, not the
Aggregator or BufferAggregator returned for a missing column, so the JSON only
contains factory properties and would be identical before this refactor. The
only missing-column test calls the on-heap factorize path for LongLast; the
five other factories and all six NIL_BUFFER_AGGREGATOR paths remain uncovered.
This lets regressions in the changed classes pass unnoticed and does not
validate the claimed fix.
**Suggestion:** Add a focused test for the actual Jackson-facing value and
cover the buffer and remaining factory variants, or narrow the production
change to code that the test demonstrably protects.
--
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]
