Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
clintropolis merged 5 commits into from
Jun 26, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23184c4
fix bug in multi-value string expression column selector
clintropolis Jun 25, 2019
dd5725b
more test
clintropolis Jun 25, 2019
e7cf510
imports!!
clintropolis Jun 25, 2019
e13869b
fixes
clintropolis Jun 25, 2019
a3b487e
Merge remote-tracking branch 'upstream/master' into expr-multi-val-se…
clintropolis Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ ExprEval applyMap(LambdaExpr expr, IndexableMapLambdaObjectBinding bindings)
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;
}
}
Expand Down
11 changes: 2 additions & 9 deletions core/src/main/java/org/apache/druid/math/expr/ExprEval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

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
Expand Down Expand Up @@ -629,7 +628,7 @@ public ExprType type()
@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
Expand All @@ -653,8 +652,6 @@ public ExprEval castTo(ExprType castTo)
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:
Expand Down Expand Up @@ -690,7 +687,7 @@ public ExprType type()
@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
Expand All @@ -714,8 +711,6 @@ public ExprEval castTo(ExprType castTo)
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:
Expand Down Expand Up @@ -788,8 +783,6 @@ public ExprEval castTo(ExprType castTo)
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:
Expand Down
11 changes: 8 additions & 3 deletions processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,13 @@ public boolean isNull()
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
Expand Down Expand Up @@ -498,7 +503,7 @@ static Supplier<Object> supplierFromObjectSelector(final BaseObjectColumnValueSe
*/
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);
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: x != null ? x.toString() : null looks more clear to me.

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more follow-up work to do, will try to revisit and clean stuff up a bit more in the future.

if (arrayVal.length > 0) {
return arrayVal;
}
Expand Down
46 changes: 46 additions & 0 deletions processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,52 @@ public void testGroupByExpressionMultiMultiAutoAuto()
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()
{
Expand Down
15 changes: 15 additions & 0 deletions processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
import org.junit.Assert;
import org.junit.Test;

import java.util.Arrays;

public class ExpressionVirtualColumnTest
{
private static final InputRow ROW0 = new MapBasedInputRow(
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -202,12 +213,16 @@ public void testMultiObjectSelector()
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
Expand Down
140 changes: 140 additions & 0 deletions sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7945,4 +7945,144 @@ public void testTimestampCeil() throws Exception
);
}


@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\"]"}
)
);
}
}
6 changes: 0 additions & 6 deletions sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -87,7 +85,6 @@
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;
Expand Down Expand Up @@ -218,9 +215,6 @@ public AuthenticationResult createEscalatedAuthenticationResult()
);

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 -> {
Expand Down