Skip to content

Constant flattening in math expression #3090

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
fjy merged 3 commits into from
Nov 14, 2016
Merged

Constant flattening in math expression #3090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d21d13
Constant flatteing in math expression
navis Jun 7, 2016
dd0c5e3
Addressed comments and fixed some bugs
navis Nov 7, 2016
0597931
Addressed comments
navis Nov 10, 2016
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
41 changes: 41 additions & 0 deletions common/src/main/java/io/druid/math/expr/Evals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,17 @@
package io.druid.math.expr;

import io.druid.common.guava.GuavaUtils;
import io.druid.java.util.common.logger.Logger;

import java.util.Arrays;
import java.util.List;

/**
*/
public class Evals
{
private static final Logger log = new Logger(Evals.class);

public static Number toNumber(Object value)
{
if (value == null) {
Expand All @@ -40,4 +46,39 @@ public static Number toNumber(Object value)
}
return longValue;
}

public static boolean isConstant(Expr expr)
{
return expr instanceof ConstantExpr;
}

public static boolean isAllConstants(Expr... exprs)
{
return isAllConstants(Arrays.asList(exprs));
}

public static boolean isAllConstants(List<Expr> exprs)
{
for (Expr expr : exprs) {
if (!(expr instanceof ConstantExpr)) {
return false;
}
}
return true;
}

// for binary operator not providing constructor of form <init>(String, Expr, Expr),
// you should create it explicitly in here
public static Expr binaryOp(BinaryOpExprBase binary, Expr left, Expr right)
{
try {
return binary.getClass()
.getDeclaredConstructor(String.class, Expr.class, Expr.class)
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jun 7, 2016

Choose a reason for hiding this comment

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

this means that every subclass of BinaryOpExprBase should have a constr matching this signature, do we need to mention this in javadoc for class BinaryOpExprBase ?

Copy link
Copy Markdown
Contributor Author

@navis navis Jun 8, 2016

Choose a reason for hiding this comment

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

I've did something like switch(op) { case X: return new Y(l, r), ... } in first. Would it be better apporoch?

Copy link
Copy Markdown
Contributor

@dclim dclim Nov 1, 2016

Choose a reason for hiding this comment

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

@navis I think this is fine, just add a comment to BinaryOpExprBase that any class that subclasses it must have a constructor with the form Class(String op, Expr left, Expr right)

Copy link
Copy Markdown
Contributor Author

@navis navis Nov 3, 2016

Choose a reason for hiding this comment

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

done

.newInstance(binary.op, left, right);
}
catch (Exception e) {
log.warn(e, "failed to rewrite expression " + binary);
return binary; // best effort.. keep it working
}
}
}
24 changes: 16 additions & 8 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 @@ -55,9 +55,9 @@ public void visit(Visitor visitor)

class LongExpr extends ConstantExpr
{
private final long value;
private final Long value;

public LongExpr(long value)
public LongExpr(Long value)
{
this.value = value;
}
Expand All @@ -71,7 +71,7 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return ExprEval.of(value);
return ExprEval.ofLong(value);
}
}

Expand Down Expand Up @@ -99,9 +99,9 @@ public ExprEval eval(ObjectBinding bindings)

class DoubleExpr extends ConstantExpr
{
private final double value;
private final Double value;

public DoubleExpr(double value)
public DoubleExpr(Double value)
{
this.value = value;
}
Expand All @@ -115,11 +115,11 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return ExprEval.of(value);
return ExprEval.ofDouble(value);
}
}

class IdentifierExpr extends ConstantExpr
class IdentifierExpr implements Expr
{
private final String value;

Expand All @@ -139,6 +139,12 @@ public ExprEval eval(ObjectBinding bindings)
{
return ExprEval.bestEffortOf(bindings.get(value));
}

@Override
public void visit(Visitor visitor)
{
visitor.visit(this);
}
}

class FunctionExpr implements Expr
Expand All @@ -161,7 +167,7 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return Parser.func.get(name.toLowerCase()).apply(args, bindings);
return Parser.getFunction(name).apply(args, bindings);
}

@Override
Expand Down Expand Up @@ -252,6 +258,8 @@ public String toString()
}
}

// all concrete subclass of this should have constructor with the form of <init>(String, Expr, Expr)
// if it's not possible, just be sure Evals.binaryOp() can handle that
abstract class BinaryOpExprBase implements Expr
{
protected final String op;
Expand Down
34 changes: 32 additions & 2 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 @@ -26,14 +26,24 @@
*/
public abstract class ExprEval<T>
{
public static ExprEval ofLong(Number longValue)
{
return new LongExprEval(longValue);
}

public static ExprEval of(long longValue)
{
return new LongExprEval(longValue);
}

public static ExprEval of(double longValue)
public static ExprEval ofDouble(Number doubleValue)
{
return new DoubleExprEval(doubleValue);
}

public static ExprEval of(double doubleValue)
{
return new DoubleExprEval(longValue);
return new DoubleExprEval(doubleValue);
}

public static ExprEval of(String stringValue)
Expand Down Expand Up @@ -108,6 +118,8 @@ public String asString()

public abstract ExprEval castTo(ExprType castTo);

public abstract Expr toExpr();

private static abstract class NumericExprEval extends ExprEval<Number> {

private NumericExprEval(Number value)
Expand Down Expand Up @@ -166,6 +178,12 @@ public final ExprEval castTo(ExprType castTo)
}
throw new IAE("invalid type " + castTo);
}

@Override
public Expr toExpr()
{
return new DoubleExpr(value == null ? null : value.doubleValue());
}
}

private static class LongExprEval extends NumericExprEval
Expand Down Expand Up @@ -200,6 +218,12 @@ public final ExprEval castTo(ExprType castTo)
}
throw new IAE("invalid type " + castTo);
}

@Override
public Expr toExpr()
{
return new LongExpr(value == null ? null : value.longValue());
}
}

private static class StringExprEval extends ExprEval<String>
Expand Down Expand Up @@ -258,5 +282,11 @@ public final ExprEval castTo(ExprType castTo)
}
throw new IAE("invalid type " + castTo);
}

@Override
public Expr toExpr()
{
return new StringExpr(value);
}
}
}
2 changes: 1 addition & 1 deletion common/src/main/java/io/druid/math/expr/ExprListenerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public void exitPowOpExpr(ExprParser.PowOpExprContext ctx)
public void exitFunctionExpr(ExprParser.FunctionExprContext ctx)
{
String fnName = ctx.getChild(0).getText();
if (!Parser.func.containsKey(fnName)) {
if (!Parser.hasFunction(fnName)) {
throw new RuntimeException("function " + fnName + " is not defined.");
}

Expand Down
5 changes: 5 additions & 0 deletions common/src/main/java/io/druid/math/expr/Function.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.joda.time.format.DateTimeFormat;
import org.joda.time.format.DateTimeFormatter;
import org.joda.time.format.ISODateTimeFormat;
import com.google.common.base.Supplier;

import java.util.List;

Expand All @@ -35,6 +36,10 @@ interface Function

ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings);

// optional interface to be used when function should be created per reference in expression
interface FunctionFactory extends Supplier<Function>, Function {
}

abstract class SingleParam implements Function
{
@Override
Expand Down
78 changes: 73 additions & 5 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 @@ -21,11 +21,13 @@


import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.collect.Lists;

import io.druid.java.util.common.IAE;
import io.druid.java.util.common.logger.Logger;
import io.druid.math.expr.antlr.ExprLexer;
import io.druid.math.expr.antlr.ExprParser;
Expand All @@ -41,16 +43,20 @@

public class Parser
{
static final Logger log = new Logger(Parser.class);
static final Map<String, Function> func;
private static final Logger log = new Logger(Parser.class);
private static final Map<String, Supplier<Function>> func;

static {
Map<String, Function> functionMap = Maps.newHashMap();
Map<String, Supplier<Function>> functionMap = Maps.newHashMap();
for (Class clazz : Function.class.getClasses()) {
if (!Modifier.isAbstract(clazz.getModifiers()) && Function.class.isAssignableFrom(clazz)) {
try {
Function function = (Function)clazz.newInstance();
functionMap.put(function.name().toLowerCase(), function);
if (function instanceof Function.FunctionFactory) {
functionMap.put(function.name().toLowerCase(), (Supplier<Function>) function);
} else {
functionMap.put(function.name().toLowerCase(), Suppliers.ofInstance(function));
}
}
catch (Exception e) {
log.info("failed to instantiate " + clazz.getName() + ".. ignoring", e);
Expand All @@ -60,7 +66,25 @@ public class Parser
func = ImmutableMap.copyOf(functionMap);
}

public static Function getFunction(String name) {
Supplier<Function> supplier = func.get(name.toLowerCase());
if (supplier == null) {
throw new IAE("Invalid function name '%s'", name);
}
return supplier.get();
}

public static boolean hasFunction(String name)
{
return func.containsKey(name.toLowerCase());
}

public static Expr parse(String in)
{
return parse(in, true);
}

public static Expr parse(String in, boolean withFlatten)
{
ExprLexer lexer = new ExprLexer(new ANTLRInputStream(in));
CommonTokenStream tokens = new CommonTokenStream(lexer);
Expand All @@ -70,7 +94,51 @@ public static Expr parse(String in)
ParseTreeWalker walker = new ParseTreeWalker();
ExprListenerImpl listener = new ExprListenerImpl(parseTree);
walker.walk(listener, parseTree);
return listener.getAST();
return withFlatten ? flatten(listener.getAST()) : listener.getAST();
}

public static Expr flatten(Expr expr)
{
if (expr instanceof BinaryOpExprBase) {
BinaryOpExprBase binary = (BinaryOpExprBase) expr;
Expr left = flatten(binary.left);
Expr right = flatten(binary.right);
if (Evals.isAllConstants(left, right)) {
expr = expr.eval(null).toExpr();
} else if (left != binary.left || right != binary.right) {
return Evals.binaryOp(binary, left, right);
}
} else if (expr instanceof UnaryExpr) {
UnaryExpr unary = (UnaryExpr) expr;
Expr eval = flatten(unary.expr);
if (eval instanceof ConstantExpr) {
expr = expr.eval(null).toExpr();
} else if (eval != unary.expr) {
if (expr instanceof UnaryMinusExpr) {
expr = new UnaryMinusExpr(eval);
} else if (expr instanceof UnaryNotExpr) {
expr = new UnaryNotExpr(eval);
} else {
expr = unary; // unknown type..
}
}
} else if (expr instanceof FunctionExpr) {
FunctionExpr functionExpr = (FunctionExpr) expr;
List<Expr> args = functionExpr.args;
boolean flattened = false;
List<Expr> flattening = Lists.newArrayListWithCapacity(args.size());
for (int i = 0; i < args.size(); i++) {
Expr flatten = flatten(args.get(i));
flattened |= flatten != args.get(i);
flattening.add(flatten);
}
if (Evals.isAllConstants(flattening)) {
expr = expr.eval(null).toExpr();
} else if (flattened) {
expr = new FunctionExpr(functionExpr.name, flattening);
}
}
return expr;
}

public static List<String> findRequiredBindings(String in)
Expand Down
Loading