From c2e5baeec352447bce34202b3014d81e8e402ecf Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 28 Dec 2016 15:31:42 -0500 Subject: [PATCH 1/4] Remove makeMathExpressionSelector from ColumnSelectorFactory. --- .../query/aggregation/AggregatorUtil.java | 27 +--- .../RowBasedColumnSelectorFactory.java | 41 +---- .../druid/segment/ColumnSelectorFactory.java | 1 - .../druid/segment/NumericColumnSelector.java | 27 ---- .../segment/QueryableIndexStorageAdapter.java | 58 ------- .../segment/incremental/IncrementalIndex.java | 7 - .../IncrementalIndexStorageAdapter.java | 55 ------- .../incremental/OnheapIncrementalIndex.java | 7 - .../virtual/ExpressionObjectSelector.java | 148 ++++++++++++++++++ .../segment/virtual/ExpressionSelectors.java | 77 +++++++++ .../aggregation/FilteredAggregatorTest.java | 7 - .../aggregation/JavaScriptAggregatorTest.java | 7 - .../TestColumnSelectorFactory.java | 7 - 13 files changed, 231 insertions(+), 238 deletions(-) delete mode 100644 processing/src/main/java/io/druid/segment/NumericColumnSelector.java create mode 100644 processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java create mode 100644 processing/src/main/java/io/druid/segment/virtual/ExpressionSelectors.java diff --git a/processing/src/main/java/io/druid/query/aggregation/AggregatorUtil.java b/processing/src/main/java/io/druid/query/aggregation/AggregatorUtil.java index e106ef828c59..6e3f8f1ec558 100644 --- a/processing/src/main/java/io/druid/query/aggregation/AggregatorUtil.java +++ b/processing/src/main/java/io/druid/query/aggregation/AggregatorUtil.java @@ -20,11 +20,12 @@ package io.druid.query.aggregation; import com.google.common.collect.Lists; +import io.druid.java.util.common.Pair; +import io.druid.math.expr.Parser; import io.druid.segment.ColumnSelectorFactory; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; -import io.druid.java.util.common.Pair; +import io.druid.segment.virtual.ExpressionSelectors; import java.util.HashSet; import java.util.LinkedList; @@ -100,16 +101,7 @@ public static FloatColumnSelector getFloatColumnSelector( return metricFactory.makeFloatColumnSelector(fieldName); } if (fieldName == null && fieldExpression != null) { - final NumericColumnSelector numeric = metricFactory.makeMathExpressionSelector(fieldExpression); - return new FloatColumnSelector() - { - @Override - public float get() - { - final Number number = numeric.get(); - return number == null ? nullValue : number.floatValue(); - } - }; + return ExpressionSelectors.makeFloatColumnSelector(metricFactory, Parser.parse(fieldExpression), nullValue); } throw new IllegalArgumentException("Must have a valid, non-null fieldName or expression"); } @@ -125,16 +117,7 @@ public static LongColumnSelector getLongColumnSelector( return metricFactory.makeLongColumnSelector(fieldName); } if (fieldName == null && fieldExpression != null) { - final NumericColumnSelector numeric = metricFactory.makeMathExpressionSelector(fieldExpression); - return new LongColumnSelector() - { - @Override - public long get() - { - final Number number = numeric.get(); - return number == null ? nullValue : number.longValue(); - } - }; + return ExpressionSelectors.makeLongColumnSelector(metricFactory, Parser.parse(fieldExpression), nullValue); } throw new IllegalArgumentException("Must have a valid, non-null fieldName or expression"); } diff --git a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java index 378543e48f87..045effbe9a34 100644 --- a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java +++ b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java @@ -22,18 +22,13 @@ import com.google.common.base.Strings; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import io.druid.data.input.Row; -import io.druid.math.expr.Evals; -import io.druid.math.expr.Expr; -import io.druid.math.expr.Parser; import io.druid.query.dimension.DimensionSpec; import io.druid.query.extraction.ExtractionFn; import io.druid.segment.ColumnSelectorFactory; import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.Column; import io.druid.segment.column.ColumnCapabilities; @@ -247,38 +242,6 @@ public Object get() } } - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - final Expr parsed = Parser.parse(expression); - - final List required = Parser.findRequiredBindings(parsed); - final Map> values = Maps.newHashMapWithExpectedSize(required.size()); - - for (final String columnName : required) { - values.put( - columnName, new Supplier() - { - @Override - public Number get() - { - return Evals.toNumber(row.get().getRaw(columnName)); - } - } - ); - } - final Expr.ObjectBinding binding = Parser.withSuppliers(values); - - return new NumericColumnSelector() - { - @Override - public Number get() - { - return parsed.eval(binding).numericValue(); - } - }; - } - @Override public ColumnCapabilities getColumnCapabilities(String columnName) { @@ -289,9 +252,7 @@ public ColumnCapabilities getColumnCapabilities(String columnName) final ValueType valueType = rowSignature.get(columnName); // Do _not_ set isDictionaryEncoded or hasBitmapIndexes, because Row-based columns do not have those things. - return valueType != null - ? new ColumnCapabilitiesImpl().setType(valueType) - : new ColumnCapabilitiesImpl().setType(ValueType.STRING); + return valueType != null ? new ColumnCapabilitiesImpl().setType(valueType) : null; } } } diff --git a/processing/src/main/java/io/druid/segment/ColumnSelectorFactory.java b/processing/src/main/java/io/druid/segment/ColumnSelectorFactory.java index afb38476c048..f550fef14e5c 100644 --- a/processing/src/main/java/io/druid/segment/ColumnSelectorFactory.java +++ b/processing/src/main/java/io/druid/segment/ColumnSelectorFactory.java @@ -31,6 +31,5 @@ public interface ColumnSelectorFactory public FloatColumnSelector makeFloatColumnSelector(String columnName); public LongColumnSelector makeLongColumnSelector(String columnName); public ObjectColumnSelector makeObjectColumnSelector(String columnName); - public NumericColumnSelector makeMathExpressionSelector(String expression); public ColumnCapabilities getColumnCapabilities(String columnName); } diff --git a/processing/src/main/java/io/druid/segment/NumericColumnSelector.java b/processing/src/main/java/io/druid/segment/NumericColumnSelector.java deleted file mode 100644 index 576211d38e48..000000000000 --- a/processing/src/main/java/io/druid/segment/NumericColumnSelector.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Licensed to Metamarkets Group Inc. (Metamarkets) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. Metamarkets 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 io.druid.segment; - -/** - */ -public interface NumericColumnSelector -{ - Number get(); -} diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index 835aa357b836..50d94f0ed79c 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -21,7 +21,6 @@ import com.google.common.base.Function; import com.google.common.base.Predicates; -import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -31,8 +30,6 @@ import io.druid.granularity.QueryGranularity; import io.druid.java.util.common.guava.Sequence; import io.druid.java.util.common.guava.Sequences; -import io.druid.math.expr.Expr; -import io.druid.math.expr.Parser; import io.druid.query.QueryInterruptedException; import io.druid.query.dimension.DimensionSpec; import io.druid.query.extraction.ExtractionFn; @@ -800,61 +797,6 @@ public Object get() }; } - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - final Expr parsed = Parser.parse(expression); - final List required = Parser.findRequiredBindings(parsed); - - final Map> values = Maps.newHashMapWithExpectedSize(required.size()); - for (String columnName : index.getColumnNames()) { - if (!required.contains(columnName)) { - continue; - } - final GenericColumn column = index.getColumn(columnName).getGenericColumn(); - if (column == null) { - continue; - } - closer.register(column); - if (column.getType() == ValueType.FLOAT) { - values.put( - columnName, new Supplier() - { - @Override - public Number get() - { - return column.getFloatSingleValueRow(cursorOffset.getOffset()); - } - } - ); - } else if (column.getType() == ValueType.LONG) { - values.put( - columnName, new Supplier() - { - @Override - public Number get() - { - return column.getLongSingleValueRow(cursorOffset.getOffset()); - } - } - ); - } else { - throw new UnsupportedOperationException( - "Not supported type " + column.getType() + " for column " + columnName - ); - } - } - final Expr.ObjectBinding binding = Parser.withSuppliers(values); - return new NumericColumnSelector() - { - @Override - public Number get() - { - return parsed.eval(binding).numericValue(); - } - }; - } - @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java index 3660099c3009..c55ff0fc3199 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java @@ -52,7 +52,6 @@ import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; import io.druid.segment.Metadata; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.Column; import io.druid.segment.column.ColumnCapabilities; @@ -167,12 +166,6 @@ public ColumnCapabilities getColumnCapabilities(String columnName) { return baseSelectorFactory.getColumnCapabilities(columnName); } - - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - return baseSelectorFactory.makeMathExpressionSelector(expression); - } }; } diff --git a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java index f52739356f5b..1b3abf46a59b 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java @@ -20,16 +20,12 @@ package io.druid.segment.incremental; import com.google.common.base.Function; -import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterators; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import io.druid.granularity.QueryGranularity; import io.druid.java.util.common.guava.Sequence; import io.druid.java.util.common.guava.Sequences; -import io.druid.math.expr.Expr; -import io.druid.math.expr.Parser; import io.druid.query.QueryInterruptedException; import io.druid.query.dimension.DefaultDimensionSpec; import io.druid.query.dimension.DimensionSpec; @@ -45,7 +41,6 @@ import io.druid.segment.LongColumnSelector; import io.druid.segment.Metadata; import io.druid.segment.NullDimensionSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.SingleScanTimeDimSelector; import io.druid.segment.StorageAdapter; @@ -62,7 +57,6 @@ import org.joda.time.Interval; import java.util.Iterator; -import java.util.List; import java.util.Map; /** @@ -553,55 +547,6 @@ public ColumnCapabilities getColumnCapabilities(String columnName) } return capabilities; } - - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - final Expr parsed = Parser.parse(expression); - - final List required = Parser.findRequiredBindings(parsed); - final Map> values = Maps.newHashMapWithExpectedSize(required.size()); - - for (String columnName : index.getMetricNames()) { - if (!required.contains(columnName)) { - continue; - } - ValueType type = index.getCapabilities(columnName).getType(); - if (type == ValueType.FLOAT) { - final int metricIndex = index.getMetricIndex(columnName); - values.put( - columnName, new Supplier() - { - @Override - public Number get() - { - return index.getMetricFloatValue(currEntry.getValue(), metricIndex); - } - } - ); - } else if (type == ValueType.LONG) { - final int metricIndex = index.getMetricIndex(columnName); - values.put( - columnName, new Supplier() - { - @Override - public Number get() - { - return index.getMetricLongValue(currEntry.getValue(), metricIndex); - } - } - ); - } - } - final Expr.ObjectBinding binding = Parser.withSuppliers(values); - return new NumericColumnSelector() { - @Override - public Number get() - { - return parsed.eval(binding).numericValue(); - } - }; - } }; } } diff --git a/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java b/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java index a278ec19bf60..7cee1bebe42e 100644 --- a/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java +++ b/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java @@ -33,7 +33,6 @@ import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; @@ -408,12 +407,6 @@ public ColumnCapabilities getColumnCapabilities(String columnName) { return delegate.getColumnCapabilities(columnName); } - - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - return delegate.makeMathExpressionSelector(expression); - } } } diff --git a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java new file mode 100644 index 000000000000..f9f025613131 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java @@ -0,0 +1,148 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.virtual; + +import com.google.common.base.Preconditions; +import com.google.common.base.Supplier; +import com.google.common.collect.Maps; +import com.google.common.primitives.Doubles; +import io.druid.common.guava.GuavaUtils; +import io.druid.math.expr.Expr; +import io.druid.math.expr.Parser; +import io.druid.segment.ColumnSelectorFactory; +import io.druid.segment.FloatColumnSelector; +import io.druid.segment.LongColumnSelector; +import io.druid.segment.ObjectColumnSelector; +import io.druid.segment.column.ColumnCapabilities; +import io.druid.segment.column.ValueType; + +import java.util.Map; + +public class ExpressionObjectSelector implements ObjectColumnSelector +{ + private final Expr expression; + private final Expr.ObjectBinding bindings; + + private ExpressionObjectSelector(Expr.ObjectBinding bindings, Expr expression) + { + this.bindings = Preconditions.checkNotNull(bindings, "bindings"); + this.expression = Preconditions.checkNotNull(expression, "expression"); + } + + public static ExpressionObjectSelector from(ColumnSelectorFactory columnSelectorFactory, Expr expression) + { + return new ExpressionObjectSelector(createBindings(columnSelectorFactory, expression), expression); + } + + private static Expr.ObjectBinding createBindings(ColumnSelectorFactory columnSelectorFactory, Expr expression) + { + final Map> suppliers = Maps.newHashMap(); + for (String columnName : Parser.findRequiredBindings(expression)) { + final ColumnCapabilities columnCapabilities = columnSelectorFactory.getColumnCapabilities(columnName); + final ValueType nativeType = columnCapabilities != null ? columnCapabilities.getType() : null; + final Supplier supplier; + + if (nativeType == ValueType.FLOAT) { + final FloatColumnSelector selector = columnSelectorFactory.makeFloatColumnSelector(columnName); + supplier = new Supplier() + { + @Override + public Number get() + { + return selector.get(); + } + }; + } else if (nativeType == ValueType.LONG) { + final LongColumnSelector selector = columnSelectorFactory.makeLongColumnSelector(columnName); + supplier = new Supplier() + { + @Override + public Number get() + { + return selector.get(); + } + }; + } else if (nativeType == null) { + // Unknown ValueType. Try making an Object selector and see if that gives us anything useful. + final ObjectColumnSelector selector = columnSelectorFactory.makeObjectColumnSelector(columnName); + final Class clazz = selector == null ? null : selector.classOfObject(); + if (selector == null || (clazz != Object.class && Number.class.isAssignableFrom(clazz))) { + // We know there are no numbers here. Use a null supplier. + supplier = null; + } else { + // There may be numbers here. + supplier = new Supplier() + { + @Override + public Number get() + { + return tryParse(selector.get()); + } + }; + } + } else { + // Unhandleable ValueType (possibly STRING or COMPLEX). + supplier = null; + } + + if (supplier != null) { + suppliers.put(columnName, supplier); + } + } + + return Parser.withSuppliers(suppliers); + } + + private static Number tryParse(final Object value) + { + if (value == null) { + return null; + } + + if (value instanceof Number) { + return (Number) value; + } + + final String stringValue = String.valueOf(value); + final Long longValue = GuavaUtils.tryParseLong(stringValue); + if (longValue != null) { + return longValue; + } + + final Double doubleValue = Doubles.tryParse(stringValue); + if (doubleValue != null) { + return doubleValue; + } + + return null; + } + + @Override + public Class classOfObject() + { + return Number.class; + } + + @Override + public Number get() + { + return expression.eval(bindings).numericValue(); + } +} diff --git a/processing/src/main/java/io/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/io/druid/segment/virtual/ExpressionSelectors.java new file mode 100644 index 000000000000..7273d95759ed --- /dev/null +++ b/processing/src/main/java/io/druid/segment/virtual/ExpressionSelectors.java @@ -0,0 +1,77 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.virtual; + +import io.druid.math.expr.Expr; +import io.druid.segment.ColumnSelectorFactory; +import io.druid.segment.FloatColumnSelector; +import io.druid.segment.LongColumnSelector; + +public class ExpressionSelectors +{ + private ExpressionSelectors() + { + // No instantiation. + } + + public static ExpressionObjectSelector makeObjectColumnSelector( + final ColumnSelectorFactory columnSelectorFactory, + final Expr expression + ) + { + return ExpressionObjectSelector.from(columnSelectorFactory, expression); + } + + public static LongColumnSelector makeLongColumnSelector( + final ColumnSelectorFactory columnSelectorFactory, + final Expr expression, + final long nullValue + ) + { + final ExpressionObjectSelector baseSelector = ExpressionObjectSelector.from(columnSelectorFactory, expression); + return new LongColumnSelector() + { + @Override + public long get() + { + final Number number = baseSelector.get(); + return number != null ? number.longValue() : nullValue; + } + }; + } + + public static FloatColumnSelector makeFloatColumnSelector( + final ColumnSelectorFactory columnSelectorFactory, + final Expr expression, + final float nullValue + ) + { + final ExpressionObjectSelector baseSelector = ExpressionObjectSelector.from(columnSelectorFactory, expression); + return new FloatColumnSelector() + { + @Override + public float get() + { + final Number number = baseSelector.get(); + return number != null ? number.floatValue() : nullValue; + } + }; + } +} diff --git a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java index 042c5ed34a68..f3a86bab175f 100644 --- a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java @@ -40,7 +40,6 @@ import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; import io.druid.segment.column.ColumnCapabilitiesImpl; @@ -183,12 +182,6 @@ public ColumnCapabilities getColumnCapabilities(String columnName) } return caps; } - - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - throw new UnsupportedOperationException(); - } }; } diff --git a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java index 3bb6d26fe65a..9c1ecb5ca982 100644 --- a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java @@ -28,7 +28,6 @@ import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; import org.junit.Assert; @@ -77,12 +76,6 @@ public ColumnCapabilities getColumnCapabilities(String columnName) { return null; } - - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - return null; - } }; static { diff --git a/processing/src/test/java/io/druid/query/groupby/epinephelinae/TestColumnSelectorFactory.java b/processing/src/test/java/io/druid/query/groupby/epinephelinae/TestColumnSelectorFactory.java index 8373698976c0..77c57c0bf3b7 100644 --- a/processing/src/test/java/io/druid/query/groupby/epinephelinae/TestColumnSelectorFactory.java +++ b/processing/src/test/java/io/druid/query/groupby/epinephelinae/TestColumnSelectorFactory.java @@ -25,7 +25,6 @@ import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NumericColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; @@ -89,12 +88,6 @@ public Object get() }; } - @Override - public NumericColumnSelector makeMathExpressionSelector(String expression) - { - throw new UnsupportedOperationException("expression is not supported in current context"); - } - @Override public ColumnCapabilities getColumnCapabilities(String columnName) { From 27b7e72d42b56e69a7c705a1a909114ebfa31f0b Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 4 Jan 2017 08:58:48 -0800 Subject: [PATCH 2/4] Add @Nullable annotations in places, fix Number.class check. --- .../groupby/RowBasedColumnSelectorFactory.java | 1 + .../druid/segment/QueryableIndexStorageAdapter.java | 2 ++ .../main/java/io/druid/segment/StorageAdapter.java | 13 +++++++++++++ .../druid/segment/incremental/IncrementalIndex.java | 1 + .../incremental/IncrementalIndexStorageAdapter.java | 2 ++ .../segment/incremental/OnheapIncrementalIndex.java | 2 ++ .../segment/virtual/ExpressionObjectSelector.java | 8 ++++---- 7 files changed, 25 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java index 045effbe9a34..c13f9f2c8669 100644 --- a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java +++ b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java @@ -242,6 +242,7 @@ public Object get() } } + @Nullable @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index 50d94f0ed79c..4750ba771dad 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -55,6 +55,7 @@ import org.joda.time.Interval; import org.roaringbitmap.IntIterator; +import javax.annotation.Nullable; import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; @@ -797,6 +798,7 @@ public Object get() }; } + @Nullable @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/StorageAdapter.java b/processing/src/main/java/io/druid/segment/StorageAdapter.java index 603c8dde7413..82b181a9bed1 100644 --- a/processing/src/main/java/io/druid/segment/StorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/StorageAdapter.java @@ -24,6 +24,7 @@ import org.joda.time.DateTime; import org.joda.time.Interval; +import javax.annotation.Nullable; import java.util.Map; /** @@ -49,7 +50,19 @@ public interface StorageAdapter extends CursorFactory public Comparable getMinValue(String column); public Comparable getMaxValue(String column); public Capabilities getCapabilities(); + + /** + * Returns capabilities of a particular column, if known. May be null if the column doesn't exist, or if + * the column does exist but the capabilities are unknown. The latter is possible with dynamically discovered + * columns. + * + * @param column column name + * + * @return capabilities, or null + */ + @Nullable public ColumnCapabilities getColumnCapabilities(String column); + public Map getDimensionHandlers(); /** diff --git a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java index c55ff0fc3199..431ebe18df67 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java @@ -161,6 +161,7 @@ public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec) return baseSelectorFactory.makeDimensionSelector(dimensionSpec); } + @Nullable @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java index 1b3abf46a59b..364445a0f1fd 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java @@ -56,6 +56,7 @@ import org.joda.time.DateTime; import org.joda.time.Interval; +import javax.annotation.Nullable; import java.util.Iterator; import java.util.Map; @@ -534,6 +535,7 @@ public Object get() } } + @Nullable @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java b/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java index 7cee1bebe42e..d5be53ae9a66 100644 --- a/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java +++ b/processing/src/main/java/io/druid/segment/incremental/OnheapIncrementalIndex.java @@ -36,6 +36,7 @@ import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; +import javax.annotation.Nullable; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -402,6 +403,7 @@ public ObjectColumnSelector makeObjectColumnSelector(String columnName) } } + @Nullable @Override public ColumnCapabilities getColumnCapabilities(String columnName) { diff --git a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java index f9f025613131..f0b673b03ec7 100644 --- a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java +++ b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java @@ -83,10 +83,7 @@ public Number get() // Unknown ValueType. Try making an Object selector and see if that gives us anything useful. final ObjectColumnSelector selector = columnSelectorFactory.makeObjectColumnSelector(columnName); final Class clazz = selector == null ? null : selector.classOfObject(); - if (selector == null || (clazz != Object.class && Number.class.isAssignableFrom(clazz))) { - // We know there are no numbers here. Use a null supplier. - supplier = null; - } else { + if (selector != null && (clazz == Object.class || Number.class.isAssignableFrom(clazz))) { // There may be numbers here. supplier = new Supplier() { @@ -96,6 +93,9 @@ public Number get() return tryParse(selector.get()); } }; + } else { + // We know there are no numbers here. Use a null supplier. + supplier = null; } } else { // Unhandleable ValueType (possibly STRING or COMPLEX). From cd15885a089a92c3081d5601757db4eab844f9c8 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 4 Jan 2017 16:11:28 -0800 Subject: [PATCH 3/4] Break up createBindings, add tests. --- .../virtual/ExpressionObjectSelector.java | 92 ++++++---- .../virtual/ExpressionObjectSelectorTest.java | 158 ++++++++++++++++++ 2 files changed, 216 insertions(+), 34 deletions(-) create mode 100644 processing/src/test/java/io/druid/segment/virtual/ExpressionObjectSelectorTest.java diff --git a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java index f0b673b03ec7..27d9e3607e8b 100644 --- a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java +++ b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java @@ -19,6 +19,7 @@ package io.druid.segment.virtual; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.Maps; @@ -33,6 +34,8 @@ import io.druid.segment.column.ColumnCapabilities; import io.druid.segment.column.ValueType; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Map; public class ExpressionObjectSelector implements ObjectColumnSelector @@ -60,43 +63,12 @@ private static Expr.ObjectBinding createBindings(ColumnSelectorFactory columnSel final Supplier supplier; if (nativeType == ValueType.FLOAT) { - final FloatColumnSelector selector = columnSelectorFactory.makeFloatColumnSelector(columnName); - supplier = new Supplier() - { - @Override - public Number get() - { - return selector.get(); - } - }; + supplier = supplierFromFloatSelector(columnSelectorFactory.makeFloatColumnSelector(columnName)); } else if (nativeType == ValueType.LONG) { - final LongColumnSelector selector = columnSelectorFactory.makeLongColumnSelector(columnName); - supplier = new Supplier() - { - @Override - public Number get() - { - return selector.get(); - } - }; + supplier = supplierFromLongSelector(columnSelectorFactory.makeLongColumnSelector(columnName)); } else if (nativeType == null) { // Unknown ValueType. Try making an Object selector and see if that gives us anything useful. - final ObjectColumnSelector selector = columnSelectorFactory.makeObjectColumnSelector(columnName); - final Class clazz = selector == null ? null : selector.classOfObject(); - if (selector != null && (clazz == Object.class || Number.class.isAssignableFrom(clazz))) { - // There may be numbers here. - supplier = new Supplier() - { - @Override - public Number get() - { - return tryParse(selector.get()); - } - }; - } else { - // We know there are no numbers here. Use a null supplier. - supplier = null; - } + supplier = supplierFromObjectSelector(columnSelectorFactory.makeObjectColumnSelector(columnName)); } else { // Unhandleable ValueType (possibly STRING or COMPLEX). supplier = null; @@ -110,6 +82,58 @@ public Number get() return Parser.withSuppliers(suppliers); } + @VisibleForTesting + @Nonnull + static Supplier supplierFromFloatSelector(final FloatColumnSelector selector) + { + return new Supplier() + { + @Override + public Number get() + { + return selector.get(); + } + }; + } + + @VisibleForTesting + @Nonnull + static Supplier supplierFromLongSelector(final LongColumnSelector selector) + { + return new Supplier() + { + @Override + public Number get() + { + return selector.get(); + } + }; + } + + @VisibleForTesting + @Nullable + static Supplier supplierFromObjectSelector(final ObjectColumnSelector selector) + { + final Class clazz = selector == null ? null : selector.classOfObject(); + if (selector != null && (clazz.isAssignableFrom(Number.class) + || clazz.isAssignableFrom(String.class) + || Number.class.isAssignableFrom(clazz))) { + // There may be numbers here. + return new Supplier() + { + @Override + public Number get() + { + return tryParse(selector.get()); + } + }; + } else { + // We know there are no numbers here. Use a null supplier. + return null; + } + } + + @Nullable private static Number tryParse(final Object value) { if (value == null) { diff --git a/processing/src/test/java/io/druid/segment/virtual/ExpressionObjectSelectorTest.java b/processing/src/test/java/io/druid/segment/virtual/ExpressionObjectSelectorTest.java new file mode 100644 index 000000000000..b2064c30dc61 --- /dev/null +++ b/processing/src/test/java/io/druid/segment/virtual/ExpressionObjectSelectorTest.java @@ -0,0 +1,158 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.virtual; + +import com.google.common.base.Supplier; +import io.druid.common.guava.SettableSupplier; +import io.druid.segment.FloatColumnSelector; +import io.druid.segment.LongColumnSelector; +import io.druid.segment.ObjectColumnSelector; +import org.junit.Assert; +import org.junit.Test; + +import java.util.List; + +public class ExpressionObjectSelectorTest +{ + @Test + public void testSupplierFromLongSelector() + { + final Supplier supplier = ExpressionObjectSelector.supplierFromLongSelector( + new LongColumnSelector() + { + @Override + public long get() + { + return 1L; + } + } + ); + + Assert.assertEquals(1L, supplier.get()); + } + + @Test + public void testSupplierFromFloatSelector() + { + final Supplier supplier = ExpressionObjectSelector.supplierFromFloatSelector( + new FloatColumnSelector() + { + @Override + public float get() + { + return 0.1f; + } + } + ); + + Assert.assertEquals(0.1f, supplier.get()); + } + + @Test + public void testSupplierFromObjectSelectorObject() + { + final SettableSupplier settableSupplier = new SettableSupplier<>(); + final Supplier supplier = ExpressionObjectSelector.supplierFromObjectSelector( + selectorFromSupplier(settableSupplier, Object.class) + ); + + Assert.assertNotNull(supplier); + Assert.assertEquals(null, supplier.get()); + + settableSupplier.set(1.1f); + Assert.assertEquals(1.1f, supplier.get()); + + settableSupplier.set(1L); + Assert.assertEquals(1L, supplier.get()); + + settableSupplier.set("1234"); + Assert.assertEquals(1234L, supplier.get()); + + settableSupplier.set("1.234"); + Assert.assertEquals(1.234d, supplier.get()); + } + + @Test + public void testSupplierFromObjectSelectorNumber() + { + final SettableSupplier settableSupplier = new SettableSupplier<>(); + final Supplier supplier = ExpressionObjectSelector.supplierFromObjectSelector( + selectorFromSupplier(settableSupplier, Number.class) + ); + + + Assert.assertNotNull(supplier); + Assert.assertEquals(null, supplier.get()); + + settableSupplier.set(1.1f); + Assert.assertEquals(1.1f, supplier.get()); + + settableSupplier.set(1L); + Assert.assertEquals(1L, supplier.get()); + } + + @Test + public void testSupplierFromObjectSelectorString() + { + final SettableSupplier settableSupplier = new SettableSupplier<>(); + final Supplier supplier = ExpressionObjectSelector.supplierFromObjectSelector( + selectorFromSupplier(settableSupplier, String.class) + ); + + Assert.assertNotNull(supplier); + Assert.assertEquals(null, supplier.get()); + + settableSupplier.set("1.1"); + Assert.assertEquals(1.1d, supplier.get()); + + settableSupplier.set("1"); + Assert.assertEquals(1L, supplier.get()); + } + + @Test + public void testSupplierFromObjectSelectorList() + { + final SettableSupplier settableSupplier = new SettableSupplier<>(); + final Supplier supplier = ExpressionObjectSelector.supplierFromObjectSelector( + selectorFromSupplier(settableSupplier, List.class) + ); + + // List can't be a number, so supplierFromObjectSelector should return null. + Assert.assertNull(supplier); + } + + private static ObjectColumnSelector selectorFromSupplier(final Supplier supplier, final Class clazz) + { + return new ObjectColumnSelector() + { + @Override + public Class classOfObject() + { + return clazz; + } + + @Override + public T get() + { + return supplier.get(); + } + }; + } +} From cd83c22b56b4ad53fcb34cebf405a5325e87dc45 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 4 Jan 2017 16:47:33 -0800 Subject: [PATCH 4/4] Add null check. --- .../java/io/druid/segment/virtual/ExpressionObjectSelector.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java index 27d9e3607e8b..239ff8a435ba 100644 --- a/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java +++ b/processing/src/main/java/io/druid/segment/virtual/ExpressionObjectSelector.java @@ -86,6 +86,7 @@ private static Expr.ObjectBinding createBindings(ColumnSelectorFactory columnSel @Nonnull static Supplier supplierFromFloatSelector(final FloatColumnSelector selector) { + Preconditions.checkNotNull(selector, "selector"); return new Supplier() { @Override @@ -100,6 +101,7 @@ public Number get() @Nonnull static Supplier supplierFromLongSelector(final LongColumnSelector selector) { + Preconditions.checkNotNull(selector, "selector"); return new Supplier() { @Override