zhang-arvin commented on PR #20070: URL: https://github.com/apache/druid/pull/20070#issuecomment-5778920521
Thanks @FrankChen021 — the review is right on all three counts, and the P1 in particular goes deeper than the diff does. Here is where I landed after tracing it. ## P1 — the change does not fix the reported failure Confirmed. `NilLongLastAggregator extends LongLastAggregator`, not `AggregatorFactory`, and `NIL_AGGREGATOR` is only ever returned from `factorize()`/`factorizeBuffered()` as a runtime object. Jackson never sees it: - `AggregatorFactory` is what carries `@JsonSubTypes` registration (`AggregatorsModule` line 139+); neither `Aggregator` nor `BufferAggregator` is registered. - `NIL_AGGREGATOR` appears exactly once outside its declaration: `return NIL_AGGREGATOR;` at line 118. - Therefore renaming the anonymous class cannot change any type id Jackson writes. This also means the type id in the report — `LongLastAggregatorFactory$1` — must have come from a **factory** subclass, not an aggregator subclass. That code path no longer exists in the tree: #7599 was actually fixed by #10332, which touched `StringFirstAggregateCombiner`/`StringLastAggregateCombiner` and `EarliestLatestAnySqlAggregator`. #7599 is already closed and merged via that PR, so the `Closes #7599` here was referencing an already-resolved issue. So this PR's premise does not hold. I did not want to silently drop the work, though, because the *reviewability* problem you identified is real, so I kept the mechanical rename (it is harmless and arguably clearer) and added the thing that was actually missing: ## P2 #3 — added the regression coverage The existing `testSerde` only deserializes a hard-coded JSON string; it never serializes, so it structurally cannot see the type id Jackson emits. Added in this PR: - **`testSerdeRoundTrip`** — serialize → assert the JSON contains no generated `$N` class name → deserialize → assert equality/name/cache key → assert a second round trip is byte-stable. - **`testNilAggregatorIsUsedForMissingColumn`** — drives the `NilColumnValueSelector` branch, asserts the shared NIL instance is reused (`assertSame`), that `aggregate()` is a no-op, and that serializing the factory never surfaces `NilLongLastAggregator`. `mvn -o -pl processing test -Dtest=LongLastAggregationTest` → 12 tests, 0 failures. I verified the new serialization assertion actually fails if an anonymous class is put on the jackson-facing surface, so it is a real guard rather than a tautology. ## P2 #2 — confirmed, and I left it out deliberately You are right that `StringFirstAggregatorFactory`, `StringLastAggregatorFactory`, `DoubleAnyAggregatorFactory`, `LongAnyAggregatorFactory` and `FloatAnyAggregatorFactory` still use anonymous NIL classes. I checked all 11 NIL-bearing factories: 6 are named (the ones this PR touches), 5 are still anonymous. I deliberately did not extend the rename to them, because on the analysis above those anonymous classes are equally unreachable from Jackson — extending the rename would multiply the diff by ~3x while fixing nothing verifiable. If you'd rather have the sweep for uniformity, say so and I'll do it in one commit; I just don't want to justify it with a bug that does not exist. Happy to close this PR instead if you judge that the right call — the test coverage is the only part with real value, and I can carry that into a fresh PR titled for what it actually does. -- 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]
