expression virtual column selector fix for expressions which produce array types (#7958)

* fix bug in multi-value string expression column selector

* more test

* imports!!

* fixes

core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java

@@ -105,10 +105,10 @@ public interface ApplyFunction
             stringsOut[i] = evaluated.asString();
             break;
           case LONG:
-            longsOut[i] = evaluated.asLong();
+            longsOut[i] = evaluated.isNumericNull() ? null : evaluated.asLong();
             break;
           case DOUBLE:
-            doublesOut[i] = evaluated.asDouble();
+            doublesOut[i] = evaluated.isNumericNull() ? null : evaluated.asDouble();
             break;
         }
       }

core/src/main/java/org/apache/druid/math/expr/ExprEval.java

@@ -26,7 +26,6 @@ import org.apache.druid.java.util.common.IAE;
 
 import javax.annotation.Nullable;
 import java.util.Arrays;
-import java.util.stream.Collectors;
 
 /**
  * Generic result holder for evaluated {@link Expr} containing the value and {@link ExprType} of the value to allow
@@ -629,7 +628,7 @@ public abstract class ExprEval<T>
     @Override
     public String[] asStringArray()
     {
-      return value == null ? null : Arrays.stream(value).map(String::valueOf).toArray(String[]::new);
+      return value == null ? null : Arrays.stream(value).map(x -> x != null ? x.toString() : null).toArray(String[]::new);
     }
 
     @Nullable
@@ -653,8 +652,6 @@ public abstract class ExprEval<T>
         return StringExprEval.OF_NULL;
       }
       switch (castTo) {
-        case STRING:
-          return ExprEval.of(Arrays.stream(value).map(String::valueOf).collect(Collectors.joining(", ")));
         case LONG_ARRAY:
           return this;
         case DOUBLE_ARRAY:
@@ -690,7 +687,7 @@ public abstract class ExprEval<T>
     @Override
     public String[] asStringArray()
     {
-      return value == null ? null : Arrays.stream(value).map(String::valueOf).toArray(String[]::new);
+      return value == null ? null : Arrays.stream(value).map(x -> x != null ? x.toString() : null).toArray(String[]::new);
     }
 
     @Nullable
@@ -714,8 +711,6 @@ public abstract class ExprEval<T>
         return StringExprEval.OF_NULL;
       }
       switch (castTo) {
-        case STRING:
-          return ExprEval.of(Arrays.stream(value).map(String::valueOf).collect(Collectors.joining(", ")));
         case LONG_ARRAY:
           return ExprEval.ofLongArray(asLongArray());
         case DOUBLE_ARRAY:
@@ -788,8 +783,6 @@ public abstract class ExprEval<T>
         return StringExprEval.OF_NULL;
       }
       switch (castTo) {
-        case STRING:
-          return ExprEval.of(Arrays.stream(value).map(String::valueOf).collect(Collectors.joining(", ")));
         case STRING_ARRAY:
           return this;
         case LONG_ARRAY:

processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java

@@ -107,8 +107,13 @@ public class ExpressionSelectors
       public Object getObject()
       {
         // No need for null check on getObject() since baseSelector impls will never return null.
-        //noinspection ConstantConditions
+        ExprEval eval = baseSelector.getObject();
-        return baseSelector.getObject().value();
+        if (eval.isArray()) {
+          return Arrays.stream(eval.asStringArray())
+                .map(NullHandling::emptyToNullIfNeeded)
+                .collect(Collectors.toList());
+        }
+        return eval.value();
       }
 
       @Override
@@ -498,7 +503,7 @@ public class ExpressionSelectors
    */
   private static Object coerceListDimToStringArray(List val)
   {
-    Object[] arrayVal = val.stream().map(Object::toString).toArray(String[]::new);
+    Object[] arrayVal = val.stream().map(x -> x != null ? x.toString() : x).toArray(String[]::new);
     if (arrayVal.length > 0) {
       return arrayVal;
     }

processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java

@@ -571,6 +571,52 @@ public class MultiValuedDimensionTest
     TestHelper.assertExpectedObjects(expectedResults, result.toList(), "expr-multi-multi-auto-auto");
   }
 
+  @Test
+  public void testGroupByExpressionMultiMultiAutoAutoWithFilter()
+  {
+    if (config.getDefaultStrategy().equals(GroupByStrategySelector.STRATEGY_V1)) {
+      expectedException.expect(RuntimeException.class);
+      expectedException.expectMessage("GroupBy v1 does not support dimension selectors with unknown cardinality.");
+    }
+    GroupByQuery query = GroupByQuery
+        .builder()
+        .setDataSource("xx")
+        .setQuerySegmentSpec(new LegacySegmentSpec("1970/3000"))
+        .setGranularity(Granularities.ALL)
+        .setDimensions(new DefaultDimensionSpec("texpr", "texpr"))
+        .setVirtualColumns(
+            new ExpressionVirtualColumn(
+                "texpr",
+                "concat(tags, othertags)",
+                ValueType.STRING,
+                TestExprMacroTable.INSTANCE
+            )
+        )
+        .setDimFilter(new SelectorDimFilter("texpr", "t1u1", null))
+        .setLimit(5)
+        .setAggregatorSpecs(new CountAggregatorFactory("count"))
+        .setContext(context)
+        .build();
+
+    Sequence<Row> result = helper.runQueryOnSegmentsObjs(
+        ImmutableList.of(
+            new QueryableIndexSegment(queryableIndex, SegmentId.dummy("sid1")),
+            new IncrementalIndexSegment(incrementalIndex, SegmentId.dummy("sid2"))
+        ),
+        query
+    );
+
+    List<Row> expectedResults = Arrays.asList(
+        GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t1u1", "count", 2L),
+        GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t1u2", "count", 2L),
+        GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t2u1", "count", 2L),
+        GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t2u2", "count", 2L),
+        GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t3u1", "count", 2L)
+    );
+
+    TestHelper.assertExpectedObjects(expectedResults, result.toList(), "expr-multi-multi-auto-auto");
+  }
+
   @Test
   public void testGroupByExpressionAuto()
   {

processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java

@@ -46,6 +46,8 @@ import org.apache.druid.segment.column.ValueType;
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.Arrays;
+
 public class ExpressionVirtualColumnTest
 {
   private static final InputRow ROW0 = new MapBasedInputRow(
@@ -93,6 +95,15 @@ public class ExpressionVirtualColumnTest
           "c", ImmutableList.of("7", "8", "9")
       )
   );
+  private static final InputRow ROWMULTI3 = new MapBasedInputRow(
+      DateTimes.of("2000-01-02T01:00:00").getMillis(),
+      ImmutableList.of(),
+      ImmutableMap.of(
+          "x", 3L,
+          "y", 4L,
+          "b", Arrays.asList(new String[]{"3", null, "5"})
+      )
+  );
 
   private static final ExpressionVirtualColumn X_PLUS_Y = new ExpressionVirtualColumn(
       "expr",
@@ -202,12 +213,16 @@ public class ExpressionVirtualColumnTest
     Assert.assertEquals(ImmutableList.of("2.0", "4.0", "6.0"), selectorImplicit.getObject());
     CURRENT_ROW.set(ROWMULTI2);
     Assert.assertEquals(ImmutableList.of("6.0", "8.0", "10.0"), selectorImplicit.getObject());
+    CURRENT_ROW.set(ROWMULTI3);
+    Assert.assertEquals(Arrays.asList("6.0", NullHandling.replaceWithDefault() ? "0.0" : null, "10.0"), selectorImplicit.getObject());
 
     final BaseObjectColumnValueSelector selectorExplicit = SCALE_LIST_EXPLICIT.makeDimensionSelector(spec, COLUMN_SELECTOR_FACTORY);
     CURRENT_ROW.set(ROWMULTI);
     Assert.assertEquals(ImmutableList.of("2.0", "4.0", "6.0"), selectorExplicit.getObject());
     CURRENT_ROW.set(ROWMULTI2);
     Assert.assertEquals(ImmutableList.of("6.0", "8.0", "10.0"), selectorExplicit.getObject());
+    CURRENT_ROW.set(ROWMULTI3);
+    Assert.assertEquals(Arrays.asList("6.0", NullHandling.replaceWithDefault() ? "0.0" : null, "10.0"), selectorExplicit.getObject());
   }
 
   @Test

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

@@ -7945,4 +7945,144 @@ public class CalciteQueryTest extends BaseCalciteQueryTest
     );
   }
 
+
+  @Test
+  public void testMultiValueStringWorksLikeStringGroupBy() throws Exception
+  {
+    List<Object[]> expected;
+    if (NullHandling.replaceWithDefault()) {
+      expected = ImmutableList.of(
+          new Object[]{"foo", 3L},
+          new Object[]{"bfoo", 2L},
+          new Object[]{"afoo", 1L},
+          new Object[]{"cfoo", 1L},
+          new Object[]{"dfoo", 1L}
+      );
+    } else {
+      expected = ImmutableList.of(
+          new Object[]{null, 2L},
+          new Object[]{"bfoo", 2L},
+          new Object[]{"afoo", 1L},
+          new Object[]{"cfoo", 1L},
+          new Object[]{"dfoo", 1L},
+          new Object[]{"foo", 1L}
+      );
+    }
+    testQuery(
+        "SELECT concat(dim3, 'foo'), SUM(cnt) FROM druid.numfoo GROUP BY 1 ORDER BY 2 DESC",
+        ImmutableList.of(
+            GroupByQuery.builder()
+                        .setDataSource(CalciteTests.DATASOURCE3)
+                        .setInterval(querySegmentSpec(Filtration.eternity()))
+                        .setGranularity(Granularities.ALL)
+                        .setVirtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING))
+                        .setDimensions(
+                            dimensions(
+                                new DefaultDimensionSpec("v0", "v0", ValueType.STRING)
+                            )
+                        )
+                        .setAggregatorSpecs(aggregators(new LongSumAggregatorFactory("a0", "cnt")))
+                        .setLimitSpec(new DefaultLimitSpec(
+                            ImmutableList.of(new OrderByColumnSpec(
+                                "a0",
+                                Direction.DESCENDING,
+                                StringComparators.NUMERIC
+                            )),
+                            Integer.MAX_VALUE
+                        ))
+                        .setContext(QUERY_CONTEXT_DEFAULT)
+                        .build()
+        ),
+        expected
+    );
+  }
+
+  @Test
+  public void testMultiValueStringWorksLikeStringGroupByWithFilter() throws Exception
+  {
+    testQuery(
+        "SELECT concat(dim3, 'foo'), SUM(cnt) FROM druid.numfoo where concat(dim3, 'foo') = 'bfoo' GROUP BY 1 ORDER BY 2 DESC",
+        ImmutableList.of(
+            GroupByQuery.builder()
+                        .setDataSource(CalciteTests.DATASOURCE3)
+                        .setInterval(querySegmentSpec(Filtration.eternity()))
+                        .setGranularity(Granularities.ALL)
+                        .setVirtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING))
+                        .setDimensions(
+                            dimensions(
+                                new DefaultDimensionSpec("v0", "v0", ValueType.STRING)
+                            )
+                        )
+                        .setDimFilter(selector("v0", "bfoo", null))
+                        .setAggregatorSpecs(aggregators(new LongSumAggregatorFactory("a0", "cnt")))
+                        .setLimitSpec(new DefaultLimitSpec(
+                            ImmutableList.of(new OrderByColumnSpec(
+                                "a0",
+                                Direction.DESCENDING,
+                                StringComparators.NUMERIC
+                            )),
+                            Integer.MAX_VALUE
+                        ))
+                        .setContext(QUERY_CONTEXT_DEFAULT)
+                        .build()
+        ),
+        ImmutableList.of(
+            new Object[]{"bfoo", 2L},
+            new Object[]{"afoo", 1L},
+            new Object[]{"cfoo", 1L}
+        )
+    );
+  }
+
+  @Test
+  public void testMultiValueStringWorksLikeStringScan() throws Exception
+  {
+    final String nullVal = NullHandling.replaceWithDefault() ? "[\"foo\"]" : "[null]";
+    testQuery(
+        "SELECT concat(dim3, 'foo') FROM druid.numfoo",
+        ImmutableList.of(
+            new Druids.ScanQueryBuilder()
+                        .dataSource(CalciteTests.DATASOURCE3)
+                        .intervals(querySegmentSpec(Filtration.eternity()))
+                        .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING))
+                        .columns(ImmutableList.of("v0"))
+                        .context(QUERY_CONTEXT_DEFAULT)
+                        .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+                        .legacy(false)
+                        .build()
+        ),
+        ImmutableList.of(
+            new Object[]{"[\"afoo\",\"bfoo\"]"},
+            new Object[]{"[\"bfoo\",\"cfoo\"]"},
+            new Object[]{"[\"dfoo\"]"},
+            new Object[]{"[\"foo\"]"},
+            new Object[]{nullVal},
+            new Object[]{nullVal}
+        )
+    );
+  }
+
+  @Test
+  public void testMultiValueStringWorksLikeStringScanWithFilter() throws Exception
+  {
+    testQuery(
+        "SELECT concat(dim3, 'foo') FROM druid.numfoo where concat(dim3, 'foo') = 'bfoo'",
+        ImmutableList.of(
+            new Druids.ScanQueryBuilder()
+                .dataSource(CalciteTests.DATASOURCE3)
+                .intervals(querySegmentSpec(Filtration.eternity()))
+                .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING))
+                .filters(selector("v0", "bfoo", null))
+                .columns(ImmutableList.of("v0"))
+                .context(QUERY_CONTEXT_DEFAULT)
+                .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+                .legacy(false)
+                .build()
+        ),
+        ImmutableList.of(
+            new Object[]{"[\"afoo\",\"bfoo\"]"},
+            new Object[]{"[\"bfoo\",\"cfoo\"]"}
+        )
+    );
+  }
 }

sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java

@@ -20,8 +20,6 @@
 package org.apache.druid.sql.calcite.util;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.google.common.base.Supplier;
-import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -87,7 +85,6 @@ import org.apache.druid.query.scan.ScanQueryEngine;
 import org.apache.druid.query.scan.ScanQueryQueryToolChest;
 import org.apache.druid.query.scan.ScanQueryRunnerFactory;
 import org.apache.druid.query.select.SelectQuery;
-import org.apache.druid.query.select.SelectQueryConfig;
 import org.apache.druid.query.select.SelectQueryEngine;
 import org.apache.druid.query.select.SelectQueryQueryToolChest;
 import org.apache.druid.query.select.SelectQueryRunnerFactory;
@@ -218,9 +215,6 @@ public class CalciteTests
   );
 
   private static final String TIMESTAMP_COLUMN = "t";
-  private static final Supplier<SelectQueryConfig> SELECT_CONFIG_SUPPLIER = Suppliers.ofInstance(
-      new SelectQueryConfig(true)
-  );
 
   private static final Injector INJECTOR = Guice.createInjector(
       (Module) binder -> {