[GitHub] [druid] cryptoe commented on a change in pull request #12210: add backwards compatibility mode for multi-value string array null value coercion

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

cryptoe commented on a change in pull request #12210:
URL: https://github.com/apache/druid/pull/12210#discussion_r794995068



##########
File path: processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java
##########
@@ -256,30 +257,43 @@ public static boolean canMapOverDictionary(
     final List<String> columns = plan.getAnalysis().getRequiredBindingsList();
     final Map<String, Pair<ExpressionType, Supplier<Object>>> suppliers = new HashMap<>();
     for (String columnName : columns) {
-      final ColumnCapabilities columnCapabilities = columnSelectorFactory.getColumnCapabilities(columnName);
-      final boolean multiVal = columnCapabilities != null && columnCapabilities.hasMultipleValues().isTrue();
+      final ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(columnName);
+      final boolean multiVal = capabilities != null && capabilities.hasMultipleValues().isTrue();
       final Supplier<Object> supplier;
-      final ExpressionType expressionType = ExpressionType.fromColumnType(columnCapabilities);
-
-      if (columnCapabilities == null ||
-          columnCapabilities.isArray() ||
-          (plan.is(ExpressionPlan.Trait.NON_SCALAR_OUTPUT) && !plan.is(ExpressionPlan.Trait.NEEDS_APPLIED))
-      ) {
-        // Unknown ValueType or array type. Try making an Object selector and see if that gives us anything useful.
+      final ExpressionType expressionType = ExpressionType.fromColumnType(capabilities);
+
+      final boolean useObjectSupplierForMultiValueStringArray =
+          capabilities != null
+          // if homogenizing null multi-value string arrays, or if a single valued function that must be applied across
+          // multi-value rows, we can just use the dimension selector, which has the homogenization behavior built-in
+          && ((!capabilities.is(ValueType.STRING))
+              || (capabilities.is(ValueType.STRING)
+                  && !ExpressionProcessing.isHomogenizeNullMultiValueStringArrays()
+                  && !plan.is(ExpressionPlan.Trait.NEEDS_APPLIED)
+              )
+          )
+          // expression has array output
+          && plan.is(ExpressionPlan.Trait.NON_SCALAR_OUTPUT);
+
+      final boolean homogenizeNullMultiValueStringArrays =

Review comment:
       Nit: Maybe move this inside the if statement at 281? 




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