You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by cw...@apache.org on 2019/06/26 23:57:19 UTC
[incubator-druid] branch master updated: expression virtual column
selector fix for expressions which produce array types (#7958)
This is an automated email from the ASF dual-hosted git repository.
cwylie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git
The following commit(s) were added to refs/heads/master by this push:
new 151edee expression virtual column selector fix for expressions which produce array types (#7958)
151edee is described below
commit 151edeec3c44da9c152d0e97c960b079edcf8328
Author: Clint Wylie <cw...@apache.org>
AuthorDate: Wed Jun 26 16:57:13 2019 -0700
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
---
.../org/apache/druid/math/expr/ApplyFunction.java | 4 +-
.../java/org/apache/druid/math/expr/ExprEval.java | 11 +-
.../druid/segment/virtual/ExpressionSelectors.java | 11 +-
.../druid/query/MultiValuedDimensionTest.java | 46 +++++++
.../virtual/ExpressionVirtualColumnTest.java | 15 +++
.../apache/druid/sql/calcite/CalciteQueryTest.java | 140 +++++++++++++++++++++
.../druid/sql/calcite/util/CalciteTests.java | 6 -
7 files changed, 213 insertions(+), 20 deletions(-)
diff --git a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
index f50fe8e..7aea6f4 100644
--- a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
+++ b/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;
}
}
diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprEval.java b/core/src/main/java/org/apache/druid/math/expr/ExprEval.java
index 1cafb17..317afdb 100644
--- a/core/src/main/java/org/apache/druid/math/expr/ExprEval.java
+++ b/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:
diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java
index c948bd0..0ef0b48 100644
--- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java
+++ b/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
- return baseSelector.getObject().value();
+ ExprEval eval = baseSelector.getObject();
+ 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;
}
diff --git a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java
index f0282c6..199019b 100644
--- a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java
+++ b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java
@@ -572,6 +572,52 @@ public class MultiValuedDimensionTest
}
@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()
{
if (config.getDefaultStrategy().equals(GroupByStrategySelector.STRATEGY_V1)) {
diff --git a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java
index 66223be..a29cc6e 100644
--- a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java
+++ b/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
diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
index 1f1e0b4..cc7a924 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
+++ b/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\"]"}
+ )
+ );
+ }
}
diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java
index cc4e935..265c6b2 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java
+++ b/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 -> {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org