Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import io.druid.query.aggregation.BufferAggregator;
import io.druid.query.aggregation.DoubleSumAggregatorFactory;
import io.druid.query.aggregation.JavaScriptAggregatorFactory;
import io.druid.query.expression.TestExprMacroTable;
import io.druid.segment.ColumnSelectorFactory;
import io.druid.segment.Cursor;
import io.druid.segment.FloatColumnSelector;
Expand Down Expand Up @@ -112,7 +113,8 @@ public void setup() throws Exception
this.expressionAggregatorFactory = new DoubleSumAggregatorFactory(
"name",
null,
"if(x>0,1.0+x,y+1)"
"if(x>0,1.0+x,y+1)",
TestExprMacroTable.INSTANCE
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ protected Map<String, Table> getTableMap()
Calcites.createRootSchema(druidSchema),
walker,
CalciteTests.createOperatorTable(),
CalciteTests.createExprMacroTable(),
plannerConfig,
new ServerConfig()
);
Expand Down
70 changes: 67 additions & 3 deletions common/src/main/java/io/druid/math/expr/Expr.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,39 @@
package io.druid.math.expr;

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 javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.List;

/**
*/
public interface Expr
{
default boolean isLiteral()
{
// Overridden by things that are literals.
return false;
}

/**
* Returns the value of expr if expr is a literal, or throws an exception otherwise.
*
* @return expr's literal value
*
* @throws IllegalStateException if expr is not a literal
*/
@Nullable
default Object getLiteralValue()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about adding LiteralExpr extending Expr and having this method? If so, other types of exprs don't have to have this method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid having instanceof checks and casts all over the place.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok.

{
// Overridden by things that are literals.
throw new ISE("Not a literal");
}

@Nonnull
ExprEval eval(ObjectBinding bindings);

interface ObjectBinding
Expand All @@ -45,6 +70,12 @@ interface Visitor

abstract class ConstantExpr implements Expr
{
@Override
public boolean isLiteral()
{
return true;
}

@Override
public void visit(Visitor visitor)
{
Expand All @@ -61,12 +92,20 @@ public LongExpr(Long value)
this.value = value;
}

@Nullable
@Override
public Object getLiteralValue()
{
return value;
}

@Override
public String toString()
{
return String.valueOf(value);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand All @@ -83,12 +122,20 @@ public StringExpr(String value)
this.value = value;
}

@Nullable
@Override
public Object getLiteralValue()
{
return value;
}

@Override
public String toString()
{
return value;
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand All @@ -105,12 +152,20 @@ public DoubleExpr(Double value)
this.value = value;
}

@Nullable
@Override
public Object getLiteralValue()
{
return value;
}

@Override
public String toString()
{
return String.valueOf(value);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand All @@ -133,6 +188,7 @@ public String toString()
return value;
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand All @@ -148,11 +204,13 @@ public void visit(Visitor visitor)

class FunctionExpr implements Expr
{
final Function function;
final String name;
final List<Expr> args;

public FunctionExpr(String name, List<Expr> args)
public FunctionExpr(Function function, String name, List<Expr> args)
{
this.function = function;
this.name = name;
this.args = args;
}
Expand All @@ -163,10 +221,11 @@ public String toString()
return "(" + name + " " + args + ")";
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
return Parser.getFunction(name).apply(args, bindings);
return function.apply(args, bindings);
}

@Override
Expand Down Expand Up @@ -203,6 +262,7 @@ class UnaryMinusExpr extends UnaryExpr
super(expr);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand Down Expand Up @@ -237,6 +297,7 @@ class UnaryNotExpr extends UnaryExpr
super(expr);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand Down Expand Up @@ -290,6 +351,7 @@ public BinaryEvalOpExprBase(String op, Expr left, Expr right)
super(op, left, right);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand Down Expand Up @@ -347,7 +409,7 @@ class BinPowExpr extends BinaryEvalOpExprBase
@Override
protected final long evalLong(long left, long right)
{
return LongMath.pow(left, (int)right);
return LongMath.pow(left, Ints.checkedCast(right));
}

@Override
Expand Down Expand Up @@ -606,6 +668,7 @@ class BinAndExpr extends BinaryOpExprBase
super(op, left, right);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand All @@ -621,6 +684,7 @@ class BinOrExpr extends BinaryOpExprBase
super(op, left, right);
}

@Nonnull
@Override
public ExprEval eval(ObjectBinding bindings)
{
Expand Down
26 changes: 18 additions & 8 deletions common/src/main/java/io/druid/math/expr/ExprListenerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package io.druid.math.expr;

import io.druid.java.util.common.RE;
import io.druid.math.expr.antlr.ExprBaseListener;
import io.druid.math.expr.antlr.ExprParser;
import org.antlr.v4.runtime.tree.ParseTree;
Expand All @@ -36,11 +37,13 @@
public class ExprListenerImpl extends ExprBaseListener
{
private final Map<ParseTree, Object> nodes;
private final ExprMacroTable macroTable;
private final ParseTree rootNodeKey;

ExprListenerImpl(ParseTree rootNodeKey)
ExprListenerImpl(ParseTree rootNodeKey, ExprMacroTable macroTable)
{
this.rootNodeKey = rootNodeKey;
this.macroTable = macroTable;
this.nodes = new HashMap<>();
}

Expand Down Expand Up @@ -285,15 +288,22 @@ public void exitPowOpExpr(ExprParser.PowOpExprContext ctx)
public void exitFunctionExpr(ExprParser.FunctionExprContext ctx)
{
String fnName = ctx.getChild(0).getText();
if (!Parser.hasFunction(fnName)) {
throw new RuntimeException("function " + fnName + " is not defined.");
final List<Expr> args = ctx.getChildCount() > 3
? (List<Expr>) nodes.get(ctx.getChild(2))
: Collections.emptyList();

Expr expr = macroTable.get(fnName, args);

if (expr == null) {
// Built-in functions.
final Function function = Parser.getFunction(fnName);
if (function == null) {
throw new RE("function '%s' is not defined.", fnName);
}
expr = new FunctionExpr(function, fnName, args);
}

List<Expr> args = ctx.getChildCount() > 3 ? (List<Expr>) nodes.get(ctx.getChild(2)) : Collections.<Expr>emptyList();
nodes.put(
ctx,
new FunctionExpr(fnName, args)
);
nodes.put(ctx, expr);
}

@Override
Expand Down
75 changes: 75 additions & 0 deletions common/src/main/java/io/druid/math/expr/ExprMacroTable.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* 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.math.expr;

import javax.annotation.Nullable;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

public class ExprMacroTable
{
private static final ExprMacroTable NIL = new ExprMacroTable(Collections.emptyList());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why "nil"? Looks more like "empty"

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jun 7, 2017

Choose a reason for hiding this comment

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

I named it nil similar to how the empty list is named nil in Lisp and in Scala, in honor of the macro systems in those languages.


private final Map<String, ExprMacro> macroMap;

public ExprMacroTable(final List<ExprMacro> macros)
{
this.macroMap = macros.stream().collect(
Collectors.toMap(
m -> m.name().toLowerCase(),
m -> m
)
);
}

public static ExprMacroTable nil()
{
return NIL;
}

/**
* Returns an expr corresponding to a function call if this table has an entry for {@code functionName}.
* Otherwise, returns null.
*
* @param functionName function name
* @param args function arguments
*
* @return expr for this function call, or null
*/
@Nullable
public Expr get(final String functionName, final List<Expr> args)
{
final ExprMacro exprMacro = macroMap.get(functionName.toLowerCase());
if (exprMacro == null) {
return null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why silently consume the situation when the function is not found? Maybe throw exception?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In ExprListenerImpl, the macro table is checked first, then built-in functions. null return makes this pattern simpler; if this threw an exception we'd have to catch it and ignore it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I annotated this with @Nullable and wrote a javadoc.

}

return exprMacro.apply(args);
}

public interface ExprMacro
{
String name();

Expr apply(final List<Expr> args);
}
}
5 changes: 0 additions & 5 deletions common/src/main/java/io/druid/math/expr/Function.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
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 @@ -36,10 +35,6 @@ 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
Loading