From 8b53f0bd21a1d6f13fe7ae5e8dbc41ae62d56d74 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 2 May 2020 17:09:36 -0700 Subject: [PATCH 1/6] Fix filter on boolean value in Transform --- .../common/granularity/PeriodGranularity.java | 2 +- .../java/org/apache/druid/math/expr/Expr.java | 1 - .../org/apache/druid/math/expr/ExprEval.java | 32 ++ .../org/apache/druid/math/expr/Function.java | 7 +- .../apache/druid/math/expr/FunctionTest.java | 14 + .../filter/PredicateValueMatcherFactory.java | 9 +- .../segment/virtual/ExpressionSelectors.java | 1 - .../PredicateValueMatcherFactoryTest.java | 428 ++++++++++++++++++ .../selector/TestColumnValueSelector.java | 179 ++++++++ .../segment/transform/TransformerTest.java | 219 +++++++++ 10 files changed, 883 insertions(+), 9 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java create mode 100644 processing/src/test/java/org/apache/druid/segment/selector/TestColumnValueSelector.java create mode 100644 processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java diff --git a/core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java b/core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java index ea0ec679e15b..0bbc1fcd2599 100644 --- a/core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java +++ b/core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java @@ -60,7 +60,7 @@ public PeriodGranularity( ) { this.period = Preconditions.checkNotNull(period, "period can't be null!"); - Preconditions.checkArgument(!Period.ZERO.equals(period), "zero period is not acceptable in QueryGranularity!"); + Preconditions.checkArgument(!Period.ZERO.equals(period), "zero period is not acceptable in PeriodGranularity!"); this.chronology = tz == null ? ISOChronology.getInstanceUTC() : ISOChronology.getInstance(tz); if (origin == null) { // default to origin in given time zone when aligning multi-period granularities diff --git a/core/src/main/java/org/apache/druid/math/expr/Expr.java b/core/src/main/java/org/apache/druid/math/expr/Expr.java index 7ec75f9e10d7..2911c7f95342 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Expr.java +++ b/core/src/main/java/org/apache/druid/math/expr/Expr.java @@ -1287,7 +1287,6 @@ public ExprEval eval(ObjectBinding bindings) return ExprEval.of(null); } - if (leftVal.type() == ExprType.STRING && rightVal.type() == ExprType.STRING) { return evalString(leftVal.asString(), rightVal.asString()); } else if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) { 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 a993790963ac..3ea6adbf65b7 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 @@ -137,6 +137,23 @@ public T value() return value; } + void setStringValue(@Nullable String value) + { + stringValue = value; + stringValueValid = true; + } + + @Nullable + String getStringValue() + { + return stringValue; + } + + boolean isStringValueValid() + { + return stringValueValid; + } + @Nullable public String asString() { @@ -567,6 +584,21 @@ private ArrayExprEval(@Nullable T[] value) super(value); } + @Override + @Nullable + public String asString() + { + if (!isStringValueValid()) { + if (value == null) { + setStringValue(null); + } else { + setStringValue(Arrays.toString(value)); + } + } + + return getStringValue(); + } + @Override public boolean isNumericNull() { diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index af52555793b7..04bba90b5b8f 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -25,6 +25,7 @@ import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.RE; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.ExprEval.ArrayExprEval; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; @@ -1490,7 +1491,11 @@ public String name() @Override public ExprEval apply(List args, Expr.ObjectBinding bindings) { - final String arg = args.get(0).eval(bindings).asString(); + final ExprEval eval = args.get(0).eval(bindings); + if (eval instanceof ArrayExprEval) { + throw new IAE("Cannot apply strlen() to an array %s", eval.asString()); + } + final String arg = eval.asString(); return arg == null ? ExprEval.ofLong(NullHandling.defaultLongValue()) : ExprEval.of(arg.length()); } diff --git a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java index 2fe52490c400..b3cd529250d7 100644 --- a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java @@ -24,12 +24,17 @@ import org.apache.druid.testing.InitializedNullHandlingTest; import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import javax.annotation.Nullable; public class FunctionTest extends InitializedNullHandlingTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private Expr.ObjectBinding bindings; @Before @@ -100,6 +105,7 @@ public void testStrlen() { assertExpr("strlen(x)", 3L); assertExpr("strlen(nonexistent)", NullHandling.defaultLongValue()); + assertExprFail("strlen(a)", IllegalArgumentException.class, "Cannot apply strlen() to an array"); } @Test @@ -365,6 +371,14 @@ private void assertExpr(final String expression, @Nullable final Object expected Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } + private void assertExprFail(final String expression, Class expectedExceptionClass, String expectedMessage) + { + final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); + expectedException.expect(expectedExceptionClass); + expectedException.expectMessage(expectedMessage); + expr.eval(bindings); + } + private void assertArrayExpr(final String expression, @Nullable final Object[] expectedResult) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/segment/filter/PredicateValueMatcherFactory.java b/processing/src/main/java/org/apache/druid/segment/filter/PredicateValueMatcherFactory.java index 9e4f1b045d52..b7f3f5dd1dca 100644 --- a/processing/src/main/java/org/apache/druid/segment/filter/PredicateValueMatcherFactory.java +++ b/processing/src/main/java/org/apache/druid/segment/filter/PredicateValueMatcherFactory.java @@ -116,11 +116,13 @@ public boolean matches() } else if (rowValue instanceof Number) { // Double or some other non-int, non-long, non-float number. return getDoublePredicate().applyDouble((double) rowValue); - } else if (rowValue instanceof String || rowValue instanceof List) { - // String or list-of-something. Cast to list of strings and evaluate them as strings. + } else { + // Other types. Cast to list of strings and evaluate them as strings. + // Boolean values are handled here as well since it is not a known type in Druid. final List rowValueStrings = Rows.objectToStrings(rowValue); if (rowValueStrings.isEmpty()) { + // Empty list is equivalent to null. return getStringPredicate().apply(null); } @@ -131,9 +133,6 @@ public boolean matches() } return false; - } else { - // Unfilterable type. Treat as null. - return getStringPredicate().apply(null); } } 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 345274aa6e74..5ae6987df88a 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 @@ -265,7 +265,6 @@ class DefaultExpressionDimensionSelector extends BaseSingleValueDimensionSelecto @Override protected String getValue() { - return NullHandling.emptyToNullIfNeeded(baseSelector.getObject().asString()); } diff --git a/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java b/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java new file mode 100644 index 000000000000..12df51d3c362 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java @@ -0,0 +1,428 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.segment.filter; + +import com.google.common.collect.ImmutableList; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.SelectorPredicateFactory; +import org.apache.druid.query.filter.ValueMatcher; +import org.apache.druid.segment.DimensionSelector; +import org.apache.druid.segment.SimpleAscendingOffset; +import org.apache.druid.segment.column.ValueType; +import org.apache.druid.segment.data.GenericIndexed; +import org.apache.druid.segment.data.VSizeColumnarInts; +import org.apache.druid.segment.data.VSizeColumnarMultiInts; +import org.apache.druid.segment.selector.TestColumnValueSelector; +import org.apache.druid.segment.serde.DictionaryEncodedColumnSupplier; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.junit.Assert; +import org.junit.Test; + +import javax.annotation.Nullable; +import java.util.Arrays; + +public class PredicateValueMatcherFactoryTest extends InitializedNullHandlingTest +{ + @Test + public void testDefaultType() + { + Assert.assertEquals(ValueType.COMPLEX, forSelector(null).defaultType()); + } + + @Test + public void testDimensionProcessorSingleValuedDimensionMatchingValue() + { + final ValueMatcher matcher = forSelector("0").makeDimensionProcessor(DimensionSelector.constant("0"), false); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testDimensionProcessorSingleValuedDimensionNotMatchingValue() + { + final ValueMatcher matcher = forSelector("1").makeDimensionProcessor(DimensionSelector.constant("0"), false); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testDimensionProcessorMultiValuedDimensionMatchingValue() + { + // Emulate multi-valued dimension + final DictionaryEncodedColumnSupplier columnSupplier = new DictionaryEncodedColumnSupplier( + GenericIndexed.fromIterable(ImmutableList.of("v1", "v2", "v3"), GenericIndexed.STRING_STRATEGY), + null, + () -> VSizeColumnarMultiInts.fromIterable(ImmutableList.of(VSizeColumnarInts.fromArray(new int[]{1}))), + 0 + ); + final ValueMatcher matcher = forSelector("v2") + .makeDimensionProcessor(columnSupplier.get().makeDimensionSelector(new SimpleAscendingOffset(1), null), true); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testDimensionProcessorMultiValuedDimensionNotMatchingValue() + { + // Emulate multi-valued dimension + final DictionaryEncodedColumnSupplier columnSupplier = new DictionaryEncodedColumnSupplier( + GenericIndexed.fromIterable(ImmutableList.of("v1", "v2", "v3"), GenericIndexed.STRING_STRATEGY), + null, + () -> VSizeColumnarMultiInts.fromIterable(ImmutableList.of(VSizeColumnarInts.fromArray(new int[]{1}))), + 0 + ); + final ValueMatcher matcher = forSelector("v3") + .makeDimensionProcessor(columnSupplier.get().makeDimensionSelector(new SimpleAscendingOffset(1), null), true); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testFloatProcessorMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Float.class, + ImmutableList.of(2.f), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("2.f").makeFloatProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testFloatProcessorNotMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Float.class, + ImmutableList.of(2.f), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("5.f").makeFloatProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testDoubleProcessorMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Double.class, + ImmutableList.of(2.), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("2.").makeDoubleProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testDoubleProcessorNotMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Double.class, + ImmutableList.of(2.), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("5.").makeDoubleProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testLongProcessorMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Long.class, + ImmutableList.of(2L), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("2").makeLongProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testLongProcessorNotMatchingValue() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Long.class, + ImmutableList.of(2L), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("5").makeLongProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingNull() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + Arrays.asList(null, "v"), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector(null).makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorEmptyString() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + Arrays.asList("", "v"), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector(null).makeComplexProcessor(columnValueSelector); + if (NullHandling.sqlCompatible()) { + Assert.assertFalse(matcher.matches()); + } else { + Assert.assertTrue(matcher.matches()); + } + } + + @Test + public void testComplexProcessorMatchingInteger() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Integer.class, + ImmutableList.of(11), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingInteger() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Integer.class, + ImmutableList.of(15), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingLong() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Long.class, + ImmutableList.of(11L), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingLong() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Long.class, + ImmutableList.of(15L), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingFloat() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Float.class, + ImmutableList.of(11.f), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11.f").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingFloat() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Float.class, + ImmutableList.of(15.f), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11.f").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingDouble() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Double.class, + ImmutableList.of(11.d), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11.d").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingDouble() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + Double.class, + ImmutableList.of(15.d), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("11.d").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingString() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of("val"), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("val").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingString() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of("bar"), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("val").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingStringList() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(ImmutableList.of("val")), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("val").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingStringList() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(ImmutableList.of("bar")), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("val").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingEmptyList() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(ImmutableList.of()), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector(null).makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingBoolean() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(false), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("false").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingBoolean() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(true), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("false").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + @Test + public void testComplexProcessorMatchingByteArray() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(StringUtils.toUtf8("var")), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("dmFy").makeComplexProcessor(columnValueSelector); + Assert.assertTrue(matcher.matches()); + } + + @Test + public void testComplexProcessorNotMatchingByteArray() + { + final TestColumnValueSelector columnValueSelector = TestColumnValueSelector.of( + String.class, + ImmutableList.of(StringUtils.toUtf8("var")), + DateTimes.nowUtc() + ); + columnValueSelector.advance(); + final ValueMatcher matcher = forSelector("val").makeComplexProcessor(columnValueSelector); + Assert.assertFalse(matcher.matches()); + } + + private static PredicateValueMatcherFactory forSelector(@Nullable String value) + { + return new PredicateValueMatcherFactory(new SelectorPredicateFactory(value)); + } +} diff --git a/processing/src/test/java/org/apache/druid/segment/selector/TestColumnValueSelector.java b/processing/src/test/java/org/apache/druid/segment/selector/TestColumnValueSelector.java new file mode 100644 index 000000000000..8e3ad9c64d4c --- /dev/null +++ b/processing/src/test/java/org/apache/druid/segment/selector/TestColumnValueSelector.java @@ -0,0 +1,179 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.segment.selector; + +import org.apache.druid.query.dimension.DimensionSpec; +import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import org.apache.druid.segment.ColumnSelectorFactory; +import org.apache.druid.segment.ColumnValueSelector; +import org.apache.druid.segment.Cursor; +import org.apache.druid.segment.DimensionSelector; +import org.apache.druid.segment.column.ColumnCapabilities; +import org.joda.time.DateTime; + +import javax.annotation.Nullable; +import java.util.Collection; +import java.util.Iterator; +import java.util.function.Supplier; +import java.util.stream.Stream; + +public class TestColumnValueSelector implements ColumnValueSelector, Cursor +{ + private final Class clazz; + private final Supplier> iteratorSupplier; + private final DateTime time; + + private Iterator iterator; + private Object value; + + public static TestColumnValueSelector of(Class clazz, Collection collection, DateTime time) + { + return new TestColumnValueSelector<>(clazz, collection::iterator, time); + } + + public static TestColumnValueSelector of(Class clazz, Stream stream, DateTime time) + { + return new TestColumnValueSelector<>(clazz, stream::iterator, time); + } + + protected TestColumnValueSelector(Class clazz, Supplier> iteratorSupplier, DateTime time) + { + this.clazz = clazz; + this.iteratorSupplier = iteratorSupplier; + this.time = time; + this.iterator = iteratorSupplier.get(); + } + + @Override + public ColumnSelectorFactory getColumnSelectorFactory() + { + return new ColumnSelectorFactory() + { + @Override + public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec) + { + throw new UnsupportedOperationException("Not implemented"); + } + + @Override + public ColumnValueSelector makeColumnValueSelector(String columnName) + { + return TestColumnValueSelector.this; + } + + @Nullable + @Override + public ColumnCapabilities getColumnCapabilities(String column) + { + return null; + } + }; + } + + @Override + public DateTime getTime() + { + return time; + } + + @Override + public void advance() + { + value = iterator.next(); + } + + @Override + public void advanceUninterruptibly() + { + advance(); + } + + @Override + public boolean isDone() + { + return !iterator.hasNext(); + } + + @Override + public boolean isDoneOrInterrupted() + { + return isDone(); + } + + @Override + public void reset() + { + iterator = iteratorSupplier.get(); + } + + @Override + public double getDouble() + { + if (value instanceof Number) { + return ((Number) value).doubleValue(); + } else { + return Double.parseDouble(value.toString()); + } + } + + @Override + public float getFloat() + { + if (value instanceof Number) { + return ((Number) value).floatValue(); + } else { + return Float.parseFloat(value.toString()); + } + } + + @Override + public long getLong() + { + if (value instanceof Number) { + return ((Number) value).longValue(); + } else { + return Long.parseLong(value.toString()); + } + } + + @Override + public void inspectRuntimeShape(RuntimeShapeInspector inspector) + { + } + + @Override + public boolean isNull() + { + return value == null; + } + + @Nullable + @Override + public Object getObject() + { + return value; + } + + @Override + public Class classOfObject() + { + return clazz; + } +} diff --git a/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java new file mode 100644 index 000000000000..7747469244ad --- /dev/null +++ b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java @@ -0,0 +1,219 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.segment.transform; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.druid.data.input.InputRow; +import org.apache.druid.data.input.InputRowListPlusRawValues; +import org.apache.druid.data.input.MapBasedInputRow; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.query.expression.TestExprMacroTable; +import org.apache.druid.query.filter.SelectorDimFilter; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.joda.time.DateTime; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class TransformerTest extends InitializedNullHandlingTest +{ + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + @Test + public void testTransformNullRowReturnNull() + { + final Transformer transformer = new Transformer(new TransformSpec(null, null)); + Assert.assertNull(transformer.transform((InputRow) null)); + Assert.assertNull(transformer.transform((InputRowListPlusRawValues) null)); + } + + @Test + public void testTransformTimeColumn() + { + final Transformer transformer = new Transformer( + new TransformSpec( + null, + ImmutableList.of( + new ExpressionTransform("__time", "timestamp_shift(__time, 'P1D', -2)", TestExprMacroTable.INSTANCE) + ) + ) + ); + final DateTime now = DateTimes.nowUtc(); + final InputRow row = new MapBasedInputRow( + now, + ImmutableList.of("dim"), + ImmutableMap.of("__time", now, "dim", false) + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(now.minusDays(2), actual.getTimestamp()); + } + + @Test + public void testTransformWithStringTransformOnBooleanColumnTransformAfterCasting() + { + final Transformer transformer = new Transformer( + new TransformSpec( + null, + ImmutableList.of(new ExpressionTransform("dim", "strlen(dim)", TestExprMacroTable.INSTANCE)) + ) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", false) + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); + Assert.assertEquals(5L, actual.getRaw("dim")); + Assert.assertEquals(row.getTimestamp(), actual.getTimestamp()); + } + + @Test + public void testTransformWithStringTransformOnLongColumnTransformAfterCasting() + { + final Transformer transformer = new Transformer( + new TransformSpec( + null, + ImmutableList.of(new ExpressionTransform("dim", "strlen(dim)", TestExprMacroTable.INSTANCE)) + ) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", 10L) + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); + Assert.assertEquals(2L, actual.getRaw("dim")); + Assert.assertEquals(row.getTimestamp(), actual.getTimestamp()); + } + + @Test + public void testTransformWithStringTransformOnDoubleColumnTransformAfterCasting() + { + final Transformer transformer = new Transformer( + new TransformSpec( + null, + ImmutableList.of(new ExpressionTransform("dim", "strlen(dim)", TestExprMacroTable.INSTANCE)) + ) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", 200.5d) + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); + Assert.assertEquals(5L, actual.getRaw("dim")); + Assert.assertEquals(row.getTimestamp(), actual.getTimestamp()); + } + + @Test + public void testTransformWithStringTransformOnListColumnThrowingException() + { + final Transformer transformer = new Transformer( + new TransformSpec( + null, + ImmutableList.of(new ExpressionTransform("dim", "strlen(dim)", TestExprMacroTable.INSTANCE)) + ) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", ImmutableList.of(10, 20, 100)) + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); + // Unlike for querying, Druid doesn't explode multi-valued columns automatically for ingestion. + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Cannot apply strlen() to an array"); + actual.getRaw("dim"); + } + + @Test + public void testTransformWithSelectorFilterWithStringBooleanValueOnBooleanColumnFilterAfterCasting() + { + final Transformer transformer = new Transformer( + new TransformSpec(new SelectorDimFilter("dim", "false", null), null) + ); + final InputRow row1 = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", false) + ); + Assert.assertEquals(row1, transformer.transform(row1)); + final InputRow row2 = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", true) + ); + Assert.assertNull(transformer.transform(row2)); + } + + @Test + public void testTransformWithSelectorFilterWithStringBooleanValueOnStringColumn() + { + final Transformer transformer = new Transformer( + new TransformSpec(new SelectorDimFilter("dim", "false", null), null) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", "false") + ); + Assert.assertEquals(row, transformer.transform(row)); + final InputRow row2 = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", "true") + ); + Assert.assertNull(transformer.transform(row2)); + } + + @Test + public void testTransformWithTransformAndFilterTransformFirst() + { + final Transformer transformer = new Transformer( + new TransformSpec( + new SelectorDimFilter("dim", "0", null), + // A boolean expression returns a long. + ImmutableList.of(new ExpressionTransform("dim", "strlen(dim) == 10", TestExprMacroTable.INSTANCE)) + ) + ); + final InputRow row = new MapBasedInputRow( + DateTimes.nowUtc(), + ImmutableList.of("dim"), + ImmutableMap.of("dim", "short") + ); + final InputRow actual = transformer.transform(row); + Assert.assertNotNull(actual); + Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); + Assert.assertEquals(0L, actual.getRaw("dim")); + Assert.assertEquals(row.getTimestamp(), actual.getTimestamp()); + } +} From 6747ea78865d72153cd0f4eb35e4bd8c5709edf2 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 2 May 2020 18:45:06 -0700 Subject: [PATCH 2/6] assert --- .../main/java/org/apache/druid/math/expr/Function.java | 4 +--- .../java/org/apache/druid/math/expr/FunctionTest.java | 8 +++++--- .../apache/druid/segment/transform/TransformerTest.java | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 04bba90b5b8f..78371be2c86c 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -1492,9 +1492,7 @@ public String name() public ExprEval apply(List args, Expr.ObjectBinding bindings) { final ExprEval eval = args.get(0).eval(bindings); - if (eval instanceof ArrayExprEval) { - throw new IAE("Cannot apply strlen() to an array %s", eval.asString()); - } + assert !(eval instanceof ArrayExprEval); final String arg = eval.asString(); return arg == null ? ExprEval.ofLong(NullHandling.defaultLongValue()) : ExprEval.of(arg.length()); } diff --git a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java index b3cd529250d7..ad9badf594a5 100644 --- a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java @@ -105,7 +105,7 @@ public void testStrlen() { assertExpr("strlen(x)", 3L); assertExpr("strlen(nonexistent)", NullHandling.defaultLongValue()); - assertExprFail("strlen(a)", IllegalArgumentException.class, "Cannot apply strlen() to an array"); + assertExprFail("strlen(a)", AssertionError.class, null); } @Test @@ -371,11 +371,13 @@ private void assertExpr(final String expression, @Nullable final Object expected Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } - private void assertExprFail(final String expression, Class expectedExceptionClass, String expectedMessage) + private void assertExprFail(final String expression, Class expectedExceptionClass, @Nullable String expectedMessage) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); expectedException.expect(expectedExceptionClass); - expectedException.expectMessage(expectedMessage); + if (expectedMessage != null) { + expectedException.expectMessage(expectedMessage); + } expr.eval(bindings); } diff --git a/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java index 7747469244ad..89df47306d8a 100644 --- a/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java +++ b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java @@ -150,8 +150,7 @@ public void testTransformWithStringTransformOnListColumnThrowingException() Assert.assertNotNull(actual); Assert.assertEquals(ImmutableList.of("dim"), actual.getDimensions()); // Unlike for querying, Druid doesn't explode multi-valued columns automatically for ingestion. - expectedException.expect(IllegalArgumentException.class); - expectedException.expectMessage("Cannot apply strlen() to an array"); + expectedException.expect(AssertionError.class); actual.getRaw("dim"); } From abe6a6bb03fa21ee07819c01d9edb62180c06d10 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 3 May 2020 13:37:52 -0700 Subject: [PATCH 3/6] more descriptive test --- .../druid/segment/filter/PredicateValueMatcherFactoryTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java b/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java index 12df51d3c362..df5a35e5d332 100644 --- a/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/segment/filter/PredicateValueMatcherFactoryTest.java @@ -404,7 +404,8 @@ public void testComplexProcessorMatchingByteArray() DateTimes.nowUtc() ); columnValueSelector.advance(); - final ValueMatcher matcher = forSelector("dmFy").makeComplexProcessor(columnValueSelector); + final String base64Encoded = StringUtils.encodeBase64String(StringUtils.toUtf8("var")); + final ValueMatcher matcher = forSelector(base64Encoded).makeComplexProcessor(columnValueSelector); Assert.assertTrue(matcher.matches()); } From c55cbce7e7b8a71726105c8b8b28c0583382d2b9 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Mon, 4 May 2020 15:55:08 -0700 Subject: [PATCH 4/6] remove assert --- core/src/main/java/org/apache/druid/math/expr/Function.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 78371be2c86c..757451fe4f41 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -1491,9 +1491,7 @@ public String name() @Override public ExprEval apply(List args, Expr.ObjectBinding bindings) { - final ExprEval eval = args.get(0).eval(bindings); - assert !(eval instanceof ArrayExprEval); - final String arg = eval.asString(); + final String arg = args.get(0).eval(bindings).asString(); return arg == null ? ExprEval.ofLong(NullHandling.defaultLongValue()) : ExprEval.of(arg.length()); } From a1eb51e7ff06d5621a76036b71d5ed3b96149451 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Mon, 4 May 2020 17:15:21 -0700 Subject: [PATCH 5/6] add assert for cached string; disable tests --- .../org/apache/druid/math/expr/ExprEval.java | 25 ++++++++++--------- .../org/apache/druid/math/expr/Function.java | 1 - .../apache/druid/math/expr/FunctionTest.java | 16 ------------ .../segment/transform/TransformerTest.java | 2 ++ 4 files changed, 15 insertions(+), 29 deletions(-) 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 3ea6adbf65b7..18d0cc75f77a 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 @@ -117,7 +117,7 @@ public static ExprEval bestEffortOf(@Nullable Object val) } // Cached String values - private boolean stringValueValid = false; + private boolean stringValueCached = false; @Nullable private String stringValue; @@ -137,34 +137,35 @@ public T value() return value; } - void setStringValue(@Nullable String value) + void cacheStringValue(@Nullable String value) { stringValue = value; - stringValueValid = true; + stringValueCached = true; } @Nullable - String getStringValue() + String getCacheStringValue() { + assert stringValueCached; return stringValue; } - boolean isStringValueValid() + boolean isStringValueCached() { - return stringValueValid; + return stringValueCached; } @Nullable public String asString() { - if (!stringValueValid) { + if (!stringValueCached) { if (value == null) { stringValue = null; } else { stringValue = String.valueOf(value); } - stringValueValid = true; + stringValueCached = true; } return stringValue; @@ -588,15 +589,15 @@ private ArrayExprEval(@Nullable T[] value) @Nullable public String asString() { - if (!isStringValueValid()) { + if (!isStringValueCached()) { if (value == null) { - setStringValue(null); + cacheStringValue(null); } else { - setStringValue(Arrays.toString(value)); + cacheStringValue(Arrays.toString(value)); } } - return getStringValue(); + return getCacheStringValue(); } @Override diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 757451fe4f41..af52555793b7 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -25,7 +25,6 @@ import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.RE; import org.apache.druid.java.util.common.StringUtils; -import org.apache.druid.math.expr.ExprEval.ArrayExprEval; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; diff --git a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java index ad9badf594a5..2fe52490c400 100644 --- a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java @@ -24,17 +24,12 @@ import org.apache.druid.testing.InitializedNullHandlingTest; import org.junit.Assert; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import javax.annotation.Nullable; public class FunctionTest extends InitializedNullHandlingTest { - @Rule - public ExpectedException expectedException = ExpectedException.none(); - private Expr.ObjectBinding bindings; @Before @@ -105,7 +100,6 @@ public void testStrlen() { assertExpr("strlen(x)", 3L); assertExpr("strlen(nonexistent)", NullHandling.defaultLongValue()); - assertExprFail("strlen(a)", AssertionError.class, null); } @Test @@ -371,16 +365,6 @@ private void assertExpr(final String expression, @Nullable final Object expected Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } - private void assertExprFail(final String expression, Class expectedExceptionClass, @Nullable String expectedMessage) - { - final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); - expectedException.expect(expectedExceptionClass); - if (expectedMessage != null) { - expectedException.expectMessage(expectedMessage); - } - expr.eval(bindings); - } - private void assertArrayExpr(final String expression, @Nullable final Object[] expectedResult) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); diff --git a/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java index 89df47306d8a..5cf1b33d5aed 100644 --- a/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java +++ b/processing/src/test/java/org/apache/druid/segment/transform/TransformerTest.java @@ -30,6 +30,7 @@ import org.apache.druid.testing.InitializedNullHandlingTest; import org.joda.time.DateTime; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -132,6 +133,7 @@ public void testTransformWithStringTransformOnDoubleColumnTransformAfterCasting( Assert.assertEquals(row.getTimestamp(), actual.getTimestamp()); } + @Ignore("Disabled until https://github.com/apache/druid/issues/9824 is fixed") @Test public void testTransformWithStringTransformOnListColumnThrowingException() { From 2309381fedf3966bfd557199a5d779bb58320358 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Mon, 4 May 2020 17:18:12 -0700 Subject: [PATCH 6/6] typo --- core/src/main/java/org/apache/druid/math/expr/ExprEval.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 18d0cc75f77a..61cdc26f6dd1 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 @@ -144,7 +144,7 @@ void cacheStringValue(@Nullable String value) } @Nullable - String getCacheStringValue() + String getCachedStringValue() { assert stringValueCached; return stringValue; @@ -597,7 +597,7 @@ public String asString() } } - return getCacheStringValue(); + return getCachedStringValue(); } @Override