Skip to content

Expressions work better with strings. #4394

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
gianm merged 5 commits into from
Jun 14, 2017
Merged

Expressions work better with strings. #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8578ea
Expressions work better with strings.
gianm Jun 6, 2017
6553f3b
Code review comments.
gianm Jun 12, 2017
5ba06db
More code review.
gianm Jun 13, 2017
3ff9516
Fix test.
gianm Jun 13, 2017
910ea69
Adjust annotations.
gianm Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 38 additions & 32 deletions common/src/main/java/io/druid/math/expr/Expr.java
View file
Open in desktop
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
Open in desktop
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"));
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Jun 13, 2017

Choose a reason for hiding this comment

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

I'm not sure about this. DoubleExpr.getLiteralValue() is nullable, which means its value can be null. Also, why is null checking needed here instead of treating nulls as 0s?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The value can't actually be null. For reasons of trying to make the behavior more consistent with Druid null handling in general, null value is always going to be a StringExprEval. (See that the constructors for DoubleExprEval and LongExprEval have a non-null check for value). I'll remove the null checks in toExpr, since they are pointless, given value cannot be null.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Jun 14, 2017

Choose a reason for hiding this comment

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

I see. Then, would you remove @Nullable annotations at DoubleExpr.getLiteralValue() and LongExpr.getLiteralValue()?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, ok. I removed those and added non-null checks to the constructors to be defensive.

}

@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"));
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Jun 13, 2017

Choose a reason for hiding this comment

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

I'm not sure about this. LongExpr.getLiteralValue() is nullable, which means its value can be null. Also, why is null checking needed here instead of treating nulls as 0s?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 13, 2017

Choose a reason for hiding this comment

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

Same response as #4394 (comment).

}

@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;
Copy link
Copy Markdown
Member

@leventov leventov Jun 12, 2017

Choose a reason for hiding this comment

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

Why consume parse errors and turn them into nulls?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 12, 2017

Choose a reason for hiding this comment

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

I thought it would be more useful to treat unparseable strings as zeroes rather than to throw errors. I don't want one bad string in a column to make the entire query fail. It's similar to the behavior of Druid's "numeric" comparator on string columns, where unparseable numbers as less than all other numbers, not as errors.

}

@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
Open in desktop
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;
Copy link
Copy Markdown
Member

@leventov leventov Jun 12, 2017

Choose a reason for hiding this comment

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

Why ignore situation when there is no binding?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Because rather than treating expressions referencing missing columns as errors, I'd like to treat them as if they were reading nulls, since that's consistent with what Druid generally does when you ask it to read columns that don't exist.

Just doing bindings::get achieves that since bindings.get(nonexistentColumnName) == null

Copy link
Copy Markdown
Member

@leventov leventov Jun 12, 2017

Choose a reason for hiding this comment

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

There is a tradeoff between "ease of use" and the problems from consuming client errors. Maybe it should be a configurable?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this needs to be configurable for expressions, since the behavior of treating nonexistent columns as having some "default" value is very ingrained in Druid -- not just in the expressions but throughout the system in general.

I guess you could argue that you want it to be configurable throughout Druid, but that is out of scope for this PR, imo. In this one I'm just trying to make Druid expression behavior better match what the rest of Druid already does.

}

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