[GitHub] [druid] clintropolis commented on a change in pull request #12078: Grouping on arrays as arrays

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

clintropolis commented on a change in pull request #12078:
URL: https://github.com/apache/druid/pull/12078#discussion_r780203919



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java
##########
@@ -128,27 +128,10 @@ public static String escapeStringLiteral(final String s)
   }
 
   /**
-   * Convert {@link RelDataType} to the most appropriate {@link ValueType}, coercing all ARRAY types to STRING (until
-   * the time is right and we are more comfortable handling Druid ARRAY types in all parts of the engine).
-   *
-   * Callers who are not scared of ARRAY types should isntead call {@link #getValueTypeForRelDataTypeFull(RelDataType)},
-   * which returns the most accurate conversion of {@link RelDataType} to {@link ValueType}.
+   * Convert {@link RelDataType} to the most appropriate {@link ValueType}}.
    */
   @Nullable
   public static ColumnType getColumnTypeForRelDataType(final RelDataType type)

Review comment:
       the engine has been waiting on array grouping for this coercion to go away, though there is a missing piece I think where we need to ensure that when grouping on a virtual column we never plan to a topn query in `DruidQuery`.
   
   I think most of the `MV_` prefixed functions have their return type as `VARCHAR` rather than `ARRAY`, so in theory the "documented" multi-value string functions shouldn't actually be affected, and any that will be is a bug that we should fix. The undocumented `ARRAY_` prefixed functions will now behave as intended and validate/operate as true SQL arrays (and so group correctly as arrays). If anyone was using the undocumented `ARRAY_` prefix functions, then I guess that behavior will change, since grouping on those expressions would previously have been grouping as `STRING` due to the implicit unnest.
   
   The only reason I can think of to add a feature flag is if need a way to hide bugs that this change causes, and that _should_ only happen if any of the `MV_` functions have bugs in their SQL output types, or if people were using the undocumented `ARRAY_` functions.




-- 
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