gianm commented on a change in pull request #12078:
URL: https://github.com/apache/druid/pull/12078#discussion_r780495521
##########
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:
As part of this patch, then, let's check all the MV functions to make
sure the return types are what we expect them to be.
As to the array stuff, one of them is documented: ARRAY_AGG. And you could
do that in a subquery and group on the result. Some questions about that:
- Do we have tests for the case of grouping on the result of an ARRAY_AGG?
If not we should add it.
- Does the behavior change or does this introduce excess risk due to
potential bugs? If so, a feature flag would be a good idea.
--
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]
