jihoonson commented on a change in pull request #11853:
URL: https://github.com/apache/druid/pull/11853#discussion_r743832987
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprEval.java
##########
@@ -554,33 +367,103 @@ public static ExprEval bestEffortOf(@Nullable Object val)
if (val instanceof ExprEval) {
return (ExprEval) val;
}
+ if (val instanceof String) {
+ return new StringExprEval((String) val);
+ }
if (val instanceof Number) {
if (val instanceof Float || val instanceof Double) {
return new DoubleExprEval((Number) val);
}
return new LongExprEval((Number) val);
}
if (val instanceof Long[]) {
- return new LongArrayExprEval((Long[]) val);
+ return new ArrayExprEval(ExpressionType.LONG_ARRAY, (Long[]) val);
}
if (val instanceof Double[]) {
- return new DoubleArrayExprEval((Double[]) val);
+ return new ArrayExprEval(ExpressionType.DOUBLE_ARRAY, (Double[]) val);
}
if (val instanceof Float[]) {
- return new DoubleArrayExprEval(Arrays.stream((Float[])
val).map(Float::doubleValue).toArray(Double[]::new));
+ return new ArrayExprEval(ExpressionType.DOUBLE_ARRAY,
Arrays.stream((Float[]) val).map(Float::doubleValue).toArray());
}
if (val instanceof String[]) {
- return new StringArrayExprEval((String[]) val);
+ return new ArrayExprEval(ExpressionType.STRING_ARRAY, (String[]) val);
+ }
+ if (val instanceof Object[]) {
+ ExpressionType arrayType = findArrayType((Object[]) val);
+ if (arrayType != null) {
+ return new ArrayExprEval(arrayType, (Object[]) val);
+ }
+ // default to string if array is empty
+ return new ArrayExprEval(ExpressionType.STRING_ARRAY, (Object[]) val);
}
if (val instanceof List) {
// do not convert empty lists to arrays with a single null element here,
because that should have been done
// by the selectors preparing their ObjectBindings if necessary. If we
get to this point it was legitimately
// empty
- return bestEffortOf(coerceListToArray((List<?>) val, false));
+ NonnullPair<ExpressionType, Object[]> coerced =
coerceListToArray((List<?>) val, false);
+ if (coerced == null) {
+ return bestEffortOf(null);
+ }
Review comment:
This seems not possible since `coerced` can be null only when `val` is
null?
##########
File path:
core/src/main/java/org/apache/druid/segment/column/ObjectByteStrategy.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.column;
+
+import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
+import java.util.Comparator;
+
+/**
+ * Naming is hard. This is the core interface extracted from another interface
called ObjectStrategy that lives in
+ * 'druid-processing'. It provides basic methods for handling converting some
type of object to a binary form, reading
+ * the binary form back into an object from a {@link ByteBuffer}, and
mechanism to perform comparisons between objects.
+ *
+ * Complex types register one of these in {@link Types#registerStrategy},
which can be retrieved by the complex
+ * type name to convert values to and from binary format, and compare them.
+ *
+ * This could be recombined with 'ObjectStrategy' should these two modules be
combined.
+ */
+public interface ObjectByteStrategy<T> extends Comparator<T>
+{
+ Class<? extends T> getClazz();
+
+ /**
+ * Convert values from their underlying byte representation.
+ *
+ * Implementations of this method <i>may</i> change the given buffer's mark,
or limit, and position.
+ *
+ * Implementations of this method <i>may not</i> store the given buffer in a
field of the "deserialized" object,
+ * need to use {@link ByteBuffer#slice()}, {@link
ByteBuffer#asReadOnlyBuffer()} or {@link ByteBuffer#duplicate()} in
+ * this case.
+ *
+ * @param buffer buffer to read value from
+ * @param numBytes number of bytes used to store the value, starting at
buffer.position()
+ * @return an object created from the given byte buffer representation
+ */
+ @Nullable
+ T fromByteBuffer(ByteBuffer buffer, int numBytes);
+
+ @Nullable
+ byte[] toBytes(@Nullable T val);
Review comment:
I know this interface is just split from `ObjectStrategy`, but could it
be better if it was `void putToBuffer(ByteBuffer buffer, @Nullable T val)`? It
seems more consistent to `fromByteBuffer` and useful to avoid materializing to
a byte array when possible. This doesn't have to be done in this PR, just
thinking out loud.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprEval.java
##########
@@ -1129,6 +1032,10 @@ public String asString()
public boolean isNumericNull()
{
if (isScalar()) {
+ if (arrayType.getElementType().is(ExprType.STRING)) {
+ Number n = computeNumber((String) getScalarValue());
Review comment:
Should we cache `n` so that we don't have to compute it again for the
same row?
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/ExpressionLambdaAggregator.java
##########
@@ -89,4 +96,88 @@ public void close()
{
// nothing to close
}
+
+ /**
+ * Tries to mimic the byte serialization of {@link Types} binary methods use
to write expression values for the
+ * {@link ExpressionLambdaBufferAggregator} in an attempt to provide
consistent size limits when using the heap
+ * based algorithm.
+ */
+ @VisibleForTesting
+ public static void estimateAndCheckMaxBytes(ExprEval eval, int maxSizeBytes)
Review comment:
Maybe package-private is better.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprEval.java
##########
@@ -1141,153 +1048,159 @@ public boolean isArray()
return true;
}
- @Override
- public int asInt()
- {
- return 0;
- }
-
- @Override
- public long asLong()
- {
- return 0;
- }
-
- @Override
- public double asDouble()
- {
- return 0;
- }
-
- @Override
- public boolean asBoolean()
- {
- return false;
- }
-
- @Nullable
- @Override
- public T[] asArray()
- {
- return value;
- }
-
- @Nullable
- public T getIndex(int index)
- {
- return value == null ? null : value[index];
- }
-
- protected boolean isScalar()
- {
- return value != null && value.length == 1;
- }
-
- @Nullable
- protected T getScalarValue()
- {
- assert value != null && value.length == 1;
- return value[0];
- }
- }
-
- private static class LongArrayExprEval extends ArrayExprEval<Long>
- {
- private static final LongArrayExprEval OF_NULL = new
LongArrayExprEval(null);
-
- private LongArrayExprEval(@Nullable Long[] value)
- {
- super(value);
- }
-
- @Override
- public ExpressionType type()
- {
- return ExpressionType.LONG_ARRAY;
- }
-
@Override
public int asInt()
{
if (isScalar()) {
- Number scalar = getScalarValue();
+ Number scalar = null;
+ if (arrayType.getElementType().isNumeric()) {
+ scalar = (Number) getScalarValue();
+ } else if (arrayType.getElementType().is(ExprType.STRING)) {
+ scalar = computeNumber((String) getScalarValue());
+ }
if (scalar == null) {
assert NullHandling.replaceWithDefault();
return 0;
}
return scalar.intValue();
}
- return super.asInt();
+ return 0;
}
@Override
public long asLong()
{
if (isScalar()) {
- Number scalar = getScalarValue();
+ Number scalar = null;
+ if (arrayType.getElementType().isNumeric()) {
+ scalar = (Number) getScalarValue();
+ } else if (arrayType.getElementType().is(ExprType.STRING)) {
+ scalar = computeNumber((String) getScalarValue());
+ }
if (scalar == null) {
assert NullHandling.replaceWithDefault();
return 0;
}
return scalar.longValue();
}
- return super.asLong();
+ return 0L;
}
@Override
public double asDouble()
{
if (isScalar()) {
- Number scalar = getScalarValue();
+ Number scalar = null;
+ if (arrayType.getElementType().isNumeric()) {
+ scalar = (Number) getScalarValue();
+ } else if (arrayType.getElementType().is(ExprType.STRING)) {
+ scalar = computeNumber((String) getScalarValue());
+ }
if (scalar == null) {
assert NullHandling.replaceWithDefault();
- return 0;
+ return 0.0;
}
return scalar.doubleValue();
}
- return super.asDouble();
+ return 0.0;
}
@Override
public boolean asBoolean()
{
if (isScalar()) {
- Number scalarValue = getScalarValue();
- if (scalarValue == null) {
- assert NullHandling.replaceWithDefault();
- return false;
+ if (arrayType.getElementType().isNumeric()) {
+ Number scalarValue = (Number) getScalarValue();
+ if (scalarValue == null) {
+ assert NullHandling.replaceWithDefault();
+ return false;
+ }
+ return Evals.asBoolean(scalarValue.longValue());
+ }
+ if (arrayType.getElementType().is(ExprType.STRING)) {
+ return Evals.asBoolean((String) getScalarValue());
}
- return Evals.asBoolean(scalarValue.longValue());
}
- return super.asBoolean();
+ return false;
+ }
+
+ @Nullable
+ @Override
+ public Object[] asArray()
+ {
+ return value;
}
@Nullable
@Override
public String[] asStringArray()
{
- return value == null ? null : Arrays.stream(value).map(x -> x != null ?
x.toString() : null).toArray(String[]::new);
+ if (value != null) {
+ if (arrayType.getElementType().is(ExprType.STRING)) {
+ return Arrays.stream(value).map(v -> (String)
v).toArray(String[]::new);
+ } else if (arrayType.getElementType().isNumeric()) {
+ return Arrays.stream(value).map(x -> x != null ? x.toString() :
null).toArray(String[]::new);
+ }
+ }
+ return null;
}
@Nullable
@Override
public Long[] asLongArray()
{
- return value;
+ if (arrayType.getElementType().is(ExprType.LONG)) {
+ return Arrays.stream(value).map(v -> (Long) v).toArray(Long[]::new);
+ } else if (arrayType.getElementType().is(ExprType.DOUBLE)) {
+ return value == null ? null : Arrays.stream(value).map(v -> ((Double)
v).longValue()).toArray(Long[]::new);
+ } else if (arrayType.getElementType().is(ExprType.STRING)) {
+ return Arrays.stream(value).map(v -> {
+ if (v == null) {
+ return null;
+ }
+ Long lv = GuavaUtils.tryParseLong((String) v);
+ if (lv == null) {
+ Double d = Doubles.tryParse((String) v);
Review comment:
Is this different than `computeNumber`?
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -282,8 +246,9 @@ public ExprEval apply(LambdaExpr lambdaExpr, List<Expr>
argsExpr, Expr.ObjectBin
}
List<List<Object>> product = CartesianList.create(arrayInputs);
- CartesianMapLambdaBinding lambdaBinding = new
CartesianMapLambdaBinding(product, lambdaExpr, bindings);
- return applyMap(lambdaExpr, lambdaBinding);
+ CartesianMapLambdaBinding lambdaBinding = new
CartesianMapLambdaBinding(elementType, product, lambdaExpr, bindings);
+ ExpressionType lambdaType = lambdaExpr.getOutputType(lambdaBinding);
Review comment:
Do you need a null check for `lambdaType`?
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/ExpressionLambdaAggregator.java
##########
@@ -89,4 +96,88 @@ public void close()
{
// nothing to close
}
+
+ /**
+ * Tries to mimic the byte serialization of {@link Types} binary methods use
to write expression values for the
+ * {@link ExpressionLambdaBufferAggregator} in an attempt to provide
consistent size limits when using the heap
+ * based algorithm.
+ */
+ @VisibleForTesting
+ public static void estimateAndCheckMaxBytes(ExprEval eval, int maxSizeBytes)
Review comment:
nit: I feel perhaps there is a better place for this method since it's
nice to have size estimation methods and serialization methods in the same
class so that we won't forget the other when we modify one of them. But maybe
it's fine to be here since it's used only in this class.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java
##########
@@ -74,10 +75,12 @@ public void reduce(
final Expr expr = Parser.parse(druidExpression.getExpression(),
plannerContext.getExprMacroTable());
final ExprEval exprResult = expr.eval(
- name -> {
- // Sanity check. Bindings should not be used for a constant
expression.
- throw new UnsupportedOperationException();
- }
+ InputBindings.forFunction(
+ name -> {
+ // Sanity check. Bindings should not be used for a constant
expression.
+ throw new UnsupportedOperationException();
Review comment:
```suggestion
throw new ISE("Bindings should not be used for constant
expressions");
```
##########
File path:
processing/src/main/java/org/apache/druid/segment/RowBasedColumnSelectorFactory.java
##########
@@ -452,6 +452,6 @@ private Number getCurrentValueAsNumber()
@Override
public ColumnCapabilities getColumnCapabilities(String columnName)
{
- return getColumnCapabilities(rowSignature, columnName);
+ return getColumnCapabilities(rowSignatureSupplier.get(), columnName);
Review comment:
nit: can cache `ColumnCapabilities` and return the same object if
`rowSignature` hasn't changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
