Re: [PR] Deserialize dimensions in group by queries to their respective types when reading from their serialized format (druid)

["clintropolis (via GitHub)" <gi...@apache.org>]

clintropolis commented on code in PR #16511:
URL: https://github.com/apache/druid/pull/16511#discussion_r1634250727


##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -186,6 +188,7 @@ public static Pair<Grouper<RowBasedKey>, Accumulator<AggregateResult, ResultRow>
       @Nullable final ReferenceCountingResourceHolder<ByteBuffer> combineBufferHolder,
       final int concurrencyHint,
       final LimitedTemporaryStorage temporaryStorage,
+      final ObjectMapper jsonMapper,
       final ObjectMapper spillMapper,

Review Comment:
   should jsonMapper and spillMapper be different? I know spillMapper is usually smile, though isn't results from historicals also smile? regardless, probably worth a comment somewhere of why we have different mappers



##########
processing/src/main/java/org/apache/druid/segment/column/ObjectStrategyComplexTypeStrategy.java:
##########
@@ -39,22 +39,25 @@ public class ObjectStrategyComplexTypeStrategy<T> implements TypeStrategy<T>
   private final TypeSignature<?> typeSignature;
   @Nullable
   private final Hash.Strategy<T> hashStrategy;
+  @Nullable
+  private final Class<?> complexDimensionType;
 
   public ObjectStrategyComplexTypeStrategy(ObjectStrategy<T> objectStrategy, TypeSignature<?> signature)
   {
-    this(objectStrategy, signature, null);
+    this(objectStrategy, signature, null, null);
   }
 
   public ObjectStrategyComplexTypeStrategy(
       ObjectStrategy<T> objectStrategy,
       TypeSignature<?> signature,
-      @Nullable final Hash.Strategy<T> hashStrategy
+      @Nullable final Hash.Strategy<T> hashStrategy,
+      @Nullable final Class<?> complexDimensionType

Review Comment:
   should this just use `getClazz` of `ObjectStrategy`?



##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategy.java:
##########
@@ -216,4 +216,9 @@ default boolean equals(T a, T b)
   {
     throw DruidException.defensive("Not implemented. Check groupable() first");
   }
+
+  default Class<?> complexDimensionType()

Review Comment:
   nit: maybe `getComplexClass` or the classic `getClazz` would be a better name since it probably isn't a different type when used as a dimension (and we have the separate `groupable` to determine if it can be used as a dimension or not)



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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