[GitHub] [druid] clintropolis commented on a change in pull request #9638: refactor internal type system

[GitBox <gi...@apache.org>]

clintropolis commented on a change in pull request #9638: refactor internal type system
URL: https://github.com/apache/druid/pull/9638#discussion_r406011220
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/aggregation/PostAggregator.java
 ##########
 @@ -43,6 +45,13 @@
   @Nullable
   String getName();
 
+  String getTypeName();
 
 Review comment:
   Ah, good point, this isn't really used currently. Initially this was the _only_ method I added to `PostAggregator` in this branch, along with `getFinalizedTypeName` to `AggregatorFactory`. However since the main users of `getTypeName` on `AggregatorFactory` (besides `ComplexMetricSerde`) and the only usages of these new methods, are converting to `ValueType`, I pivoted near completion to add the additional methods that just directly return the needed thing.
   
   I left it in because I sort of have a dream that one day `ComplexMetrics` will be rebranded into `ComplexTypes` to spiritually decouple it from `Aggregators`, but will still provide a mapping of all the `getTypeName` strings for aggs (and now maybe postaggs, and anything else?) that have `ValueType.COMPLEX` for `getType` to a serde as is now, forming a centralized registry for _all_ complex types. Whether this is _actually_ useful or not I am not yet totally certain, maybe just using jackson like we do now for everything that isn't part of a segment is enough; I think I will need to think a bit more about this, and maybe get deeper into some of the follow-up work before it will become fully apparent.
   
   Should I remove it for now since it isn't really being used other than as a vessel to convert to `ValueType`? I still find it sort of useful to make it super obvious what _actual_ type is being spit out by the `PostAggregator` is since its significantly more descriptive than `ValueType.COMPLEX`, and easier than examining the output of the `compute` method closely, but I guess javadocs could accomplish the same thing.
   
   At minimum I was still planning to add javadocs to this interface, so could document how it isn't really used, if you're cool with leaving it in for now, or I can remove. I'll try to think about this some more as well as I try to wrap up this PR.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org