Re: [PR] SQL: Support GROUP BY and ORDER BY for NULL types. (druid)

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

gianm commented on code in PR #16252:
URL: https://github.com/apache/druid/pull/16252#discussion_r1600226518


##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java:
##########
@@ -244,12 +244,22 @@ public static boolean isLongType(SqlTypeName sqlTypeName)
   }
 
   /**
-   * Returns the natural StringComparator associated with the RelDataType
+   * Returns the natural StringComparator associated with the RelDataType, or null if the type is not convertible to
+   * {@link ColumnType} by {@link #getColumnTypeForRelDataType(RelDataType)}.
    */
+  @Nullable
   public static StringComparator getStringComparatorForRelDataType(RelDataType dataType)
   {
-    final ColumnType valueType = getColumnTypeForRelDataType(dataType);
-    return getStringComparatorForValueType(valueType);
+    if (dataType.getSqlTypeName() == SqlTypeName.NULL) {
+      return StringComparators.NATURAL;
+    } else {
+      final ColumnType valueType = getColumnTypeForRelDataType(dataType);

Review Comment:
   `ColumnType` is meant to represent types that the native query engine can store and process. We don't _really_ have a pure-`NULL` type there, so adding one may have larger consequences that I did not want to run into in this patch. I thought it would be better to keep the changes to the SQL layer.



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