Skip to content

Commit

Permalink
Expressions work better with strings. (apache#4394)
Browse files Browse the repository at this point in the history 
* Expressions work better with strings.

- ExpressionObjectSelector able to read from string columns, and able to
  return strings.
- ExpressionVirtualColumn able to offer string (and long for that matter)
  as its native type.
- ExpressionPostAggregator able to return strings.
- groupBy, topN: Allow post-aggregators to accept dimensions as inputs,
  making ExpressionPostAggregator more useful.
- topN: Use DimExtractionTopNAlgorithm for STRING columns that do not
  have dictionaries, allowing it to work with STRING-type expression
  virtual columns.
- Adjusts null handling to better match the rest of Druid: null and
  empty string treated the same; nulls implicitly treated as zeroes in
  numeric context.

* Code review comments.

* More code review.

* Fix test.

* Adjust annotations.
  • Loading branch information
@gianm
gianm authored Jun 14, 2017
1 parent 976492c commit 6edee7f
Show file tree
Hide file tree
Showing 26 changed files with 671 additions and 260 deletions.
70 changes: 38 additions & 32 deletions common/src/main/java/io/druid/math/expr/Expr.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@

package io.druid.math.expr;

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.math.LongMath;
import com.google.common.primitives.Ints;
import io.druid.java.util.common.IAE;
import io.druid.java.util.common.ISE;
import io.druid.java.util.common.guava.Comparators;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.List;
import java.util.Objects;

/**
*/
Expand Down Expand Up @@ -57,7 +61,8 @@ default Object getLiteralValue()

interface ObjectBinding
{
Number get(String name);
@Nullable
Object get(String name);
}

void visit(Visitor visitor);
Expand Down Expand Up @@ -89,10 +94,10 @@ class LongExpr extends ConstantExpr

public LongExpr(Long value)
{
this.value = value;
this.value = Preconditions.checkNotNull(value, "value");
}

@Nullable
@Nonnull
@Override
public Object getLiteralValue()
{
Expand All @@ -119,7 +124,7 @@ class StringExpr extends ConstantExpr

public StringExpr(String value)
{
this.value = value;
this.value = Strings.emptyToNull(value);
}

@Nullable
Expand Down Expand Up @@ -149,10 +154,10 @@ class DoubleExpr extends ConstantExpr

public DoubleExpr(Double value)
{
this.value = value;
this.value = Preconditions.checkNotNull(value, "value");
}

@Nullable
@Nonnull
@Override
public Object getLiteralValue()
{
Expand Down Expand Up @@ -357,19 +362,16 @@ public ExprEval eval(ObjectBinding bindings)
{
ExprEval leftVal = left.eval(bindings);
ExprEval rightVal = right.eval(bindings);
if (leftVal.isNull() || rightVal.isNull()) {
return ExprEval.of(null);
}
if (leftVal.type() == ExprType.STRING || rightVal.type() == ExprType.STRING) {
if (leftVal.type() == ExprType.STRING && rightVal.type() == ExprType.STRING) {
return evalString(leftVal.asString(), rightVal.asString());
}
if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) {
} else if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) {
return ExprEval.of(evalLong(leftVal.asLong(), rightVal.asLong()));
} else {
return ExprEval.of(evalDouble(leftVal.asDouble(), rightVal.asDouble()));
}
return ExprEval.of(evalDouble(leftVal.asDouble(), rightVal.asDouble()));
}

protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
throw new IllegalArgumentException("unsupported type " + ExprType.STRING);
}
Expand Down Expand Up @@ -487,9 +489,9 @@ class BinPlusExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left + right);
return ExprEval.of(Strings.nullToEmpty(left) + Strings.nullToEmpty(right));
}

@Override
Expand All @@ -513,9 +515,9 @@ class BinLtExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left.compareTo(right) < 0, ExprType.LONG);
return ExprEval.of(Comparators.<String>naturalNullsFirst().compare(left, right) < 0, ExprType.LONG);
}

@Override
Expand All @@ -527,7 +529,8 @@ protected final long evalLong(long left, long right)
@Override
protected final double evalDouble(double left, double right)
{
return Evals.asDouble(left < right);
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
}

Expand All @@ -539,9 +542,9 @@ class BinLeqExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left.compareTo(right) <= 0, ExprType.LONG);
return ExprEval.of(Comparators.<String>naturalNullsFirst().compare(left, right) <= 0, ExprType.LONG);
}

@Override
Expand All @@ -553,7 +556,8 @@ protected final long evalLong(long left, long right)
@Override
protected final double evalDouble(double left, double right)
{
return Evals.asDouble(left <= right);
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) <= 0);
}
}

Expand All @@ -565,9 +569,9 @@ class BinGtExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left.compareTo(right) > 0, ExprType.LONG);
return ExprEval.of(Comparators.<String>naturalNullsFirst().compare(left, right) > 0, ExprType.LONG);
}

@Override
Expand All @@ -579,7 +583,8 @@ protected final long evalLong(long left, long right)
@Override
protected final double evalDouble(double left, double right)
{
return Evals.asDouble(left > right);
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) > 0);
}
}

Expand All @@ -591,9 +596,9 @@ class BinGeqExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left.compareTo(right) >= 0, ExprType.LONG);
return ExprEval.of(Comparators.<String>naturalNullsFirst().compare(left, right) >= 0, ExprType.LONG);
}

@Override
Expand All @@ -605,7 +610,8 @@ protected final long evalLong(long left, long right)
@Override
protected final double evalDouble(double left, double right)
{
return Evals.asDouble(left >= right);
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) >= 0);
}
}

Expand All @@ -617,9 +623,9 @@ class BinEqExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(left.equals(right), ExprType.LONG);
return ExprEval.of(Objects.equals(left, right), ExprType.LONG);
}

@Override
Expand All @@ -643,9 +649,9 @@ class BinNeqExpr extends BinaryEvalOpExprBase
}

@Override
protected ExprEval evalString(String left, String right)
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
return ExprEval.of(!left.equals(right), ExprType.LONG);
return ExprEval.of(!Objects.equals(left, right), ExprType.LONG);
}

@Override
Expand Down
50 changes: 28 additions & 22 deletions common/src/main/java/io/druid/math/expr/ExprEval.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@

package io.druid.math.expr;

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.primitives.Doubles;
import com.google.common.primitives.Ints;
import io.druid.common.guava.GuavaUtils;
import io.druid.java.util.common.IAE;

/**
Expand Down Expand Up @@ -72,9 +76,9 @@ public static ExprEval bestEffortOf(Object val)
}
if (val instanceof Number) {
if (val instanceof Float || val instanceof Double) {
return new DoubleExprEval((Number)val);
return new DoubleExprEval((Number) val);
}
return new LongExprEval((Number)val);
return new LongExprEval((Number) val);
}
return new StringExprEval(val == null ? null : String.valueOf(val));
}
Expand All @@ -98,11 +102,6 @@ public boolean isNull()
return value == null;
}

public Number numericValue()
{
return (Number) value;
}

public abstract int asInt();

public abstract long asLong();
Expand All @@ -120,7 +119,8 @@ public String asString()

public abstract Expr toExpr();

private static abstract class NumericExprEval extends ExprEval<Number> {
private static abstract class NumericExprEval extends ExprEval<Number>
{

private NumericExprEval(Number value)
{
Expand Down Expand Up @@ -150,7 +150,7 @@ private static class DoubleExprEval extends NumericExprEval
{
private DoubleExprEval(Number value)
{
super(value);
super(Preconditions.checkNotNull(value, "value"));
}

@Override
Expand Down Expand Up @@ -182,15 +182,15 @@ public final ExprEval castTo(ExprType castTo)
@Override
public Expr toExpr()
{
return new DoubleExpr(value == null ? null : value.doubleValue());
return new DoubleExpr(value.doubleValue());
}
}

private static class LongExprEval extends NumericExprEval
{
private LongExprEval(Number value)
{
super(value);
super(Preconditions.checkNotNull(value, "value"));
}

@Override
Expand Down Expand Up @@ -222,15 +222,15 @@ public final ExprEval castTo(ExprType castTo)
@Override
public Expr toExpr()
{
return new LongExpr(value == null ? null : value.longValue());
return new LongExpr(value.longValue());
}
}

private static class StringExprEval extends ExprEval<String>
{
private StringExprEval(String value)
{
super(value);
super(Strings.emptyToNull(value));
}

@Override
Expand All @@ -239,28 +239,34 @@ public final ExprType type()
return ExprType.STRING;
}

@Override
public final boolean isNull()
{
return Strings.isNullOrEmpty(value);
}

@Override
public final int asInt()
{
return Integer.parseInt(value);
if (value == null) {
return 0;
}

final Integer theInt = Ints.tryParse(value);
return theInt == null ? 0 : theInt;
}

@Override
public final long asLong()
{
return Long.parseLong(value);
// GuavaUtils.tryParseLong handles nulls, no need for special null handling here.
final Long theLong = GuavaUtils.tryParseLong(value);
return theLong == null ? 0L : theLong;
}

@Override
public final double asDouble()
{
return Double.parseDouble(value);
if (value == null) {
return 0.0;
}

final Double theDouble = Doubles.tryParse(value);
return theDouble == null ? 0.0 : theDouble;
}

@Override
Expand Down
26 changes: 5 additions & 21 deletions common/src/main/java/io/druid/math/expr/Parser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,30 +148,14 @@ public void visit(Expr expr)

public static Expr.ObjectBinding withMap(final Map<String, ?> bindings)
{
return new Expr.ObjectBinding()
{
@Override
public Number get(String name)
{
Number number = (Number)bindings.get(name);
if (number == null && !bindings.containsKey(name)) {
throw new RuntimeException("No binding found for " + name);
}
return number;
}
};
return bindings::get;
}

public static Expr.ObjectBinding withSuppliers(final Map<String, Supplier<Number>> bindings)
public static Expr.ObjectBinding withSuppliers(final Map<String, Supplier<Object>> bindings)
{
return new Expr.ObjectBinding()
{
@Override
public Number get(String name)
{
Supplier<Number> supplier = bindings.get(name);
return supplier == null ? null : supplier.get();
}
return (String name) -> {
Supplier<Object> supplier = bindings.get(name);
return supplier == null ? null : supplier.get();
};
}
}
Loading

0 comments on commit 6edee7f

Please sign in to comment.