Expressions: Add ExprMacros. by gianm · Pull Request #4365 · apache/druid

gianm · 2017-06-05T22:39:09Z

ExprMacros have the same syntax as functions but can convert themselves
to any kind of Expr at parse-time.

ExprMacroTable is an extension point for adding new ExprMacros. Anything
that might need to parse expressions needs an ExprMacroTable, which can
be injected through Guice.

The motivation here is two fold.

Provide a way to define functions where, for efficiency, some parts of the evaluation should be done ahead of time. The like function added here is an example: its pattern is compiled into a LikeMatcher object at expression-parse-time.
Provide a way for extensions to add new functions (they can do it through defining a macro).

… but can convert themselves to any kind of Expr at parse-time. ExprMacroTable is an extension point for adding new ExprMacros. Anything that might need to parse expressions needs an ExprMacroTable, which can be injected through Guice.

gianm · 2017-06-05T22:39:42Z

Broken out from #4360.

fjy · 2017-06-06T17:11:29Z

👍

leventov · 2017-06-06T20:43:17Z

  protected final long evalLong(long left, long right)
  {
-    return LongMath.pow(left, (int)right);
+    return LongMath.pow(left, (int) right);


Maybe Ints.checkedCast() for sanity

leventov · 2017-06-06T20:44:19Z

+      // Built-in functions.
+      final Function function = Parser.getFunction(fnName);
+      if (function == null) {
+        throw new RuntimeException("function '" + fnName + "' is not defined.");


Could use Druid's RE with pattern.

leventov · 2017-06-06T20:46:13Z

+
+public class ExprMacroTable
+{
+  private static final ExprMacroTable NIL = new ExprMacroTable(Collections.emptyList());


Why "nil"? Looks more like "empty"

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.

leventov · 2017-06-06T20:47:15Z

+  {
+    final ExprMacro exprMacro = macroMap.get(functionName.toLowerCase());
+    if (exprMacro == null) {
+      return null;


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

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.

I annotated this with @Nullable and wrote a javadoc.

leventov · 2017-06-06T20:51:38Z

+  public Expr apply(final List<Expr> args)
+  {
+    if (args.size() < 2 || args.size() > 3) {
+      throw new IAE("'%s' must have 2 or 3 arguments", name());


[%s] is usually used

Changed to "Function[%s] must have 2 or 3 arguments"

leventov · 2017-06-06T20:53:20Z

+    if (escape != null && escape.length() != 1) {
+      throw new IllegalArgumentException("Escape must be null or a single character");
+    } else {
+      escapeChar = (escape == null || escape.isEmpty()) ? null : escape.charAt(0);


isEmpty() check is redundant because length must be 1, it is checked above.

Removed the redundant check.

leventov · 2017-06-06T21:11:58Z

The transient integration test failure is: #4359 (comment)

gianm · 2017-06-07T00:41:43Z

@leventov thanks for review; pushed changes.

jihoonson · 2017-06-07T02:20:00Z

+   * @throws IllegalStateException if expr is not a literal
+   */
+  @Nullable
+  default Object getLiteralValue()


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

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

jihoonson · 2017-06-07T03:01:02Z

  }

-  public static boolean hasFunction(String name)
+  public static Function getFunction(String name)


We need to check argument types maybe here for type safety in the future? I recognize nulls are currently returned for unsupported argument types, but if we're going to make Druid more SQL-compatible, we need to check types somewhere.

Yes, I think at some point, the expressions should be type-aware at parse time. It will help with performance too. I think in the future, we want to be doing code generation and that will require advance knowledge of what type everything is going to be.

For now, they aren't type-aware until runtime.

Thanks. It sounds good.

jihoonson · 2017-06-07T03:07:04Z

-    return fieldName != null ? Collections.singletonList(fieldName) : Parser.findRequiredBindings(expression);
+    return fieldName != null
+           ? Collections.singletonList(fieldName)
+           : Parser.findRequiredBindings(Parser.parse(expression, macroTable));


It looks to be possible to keep parse result if fieldName is null in the constructor. Same for other AggregatorFactorys.

It's true, but would require some refactoring of AggregatorUtil and its callers. I think it'd be better to do it in a separate patch.

jihoonson · 2017-06-07T03:16:12Z

@gianm thanks for breaking the patch into small ones. Looks good to me overall.

gianm · 2017-06-07T03:32:12Z

Thanks for the review @jihoonson.

leventov

LGTM

gianm added Design Review Feature labels Jun 5, 2017

gianm added this to the 0.10.2 milestone Jun 5, 2017

gianm changed the title ~~Expressions: Add ExprMacros, which have the same syntax as functions,~~ Expressions: Add ExprMacros. Jun 5, 2017

gianm mentioned this pull request Jun 5, 2017

SQL + Expressions = Best friends forever. #4360

Merged

gianm removed this from the 0.10.2 milestone Jun 6, 2017

Merge branch 'master' into se-expr-macros

56582cf

leventov requested changes Jun 6, 2017

View reviewed changes

Address code review comments.

6d6986d

jihoonson reviewed Jun 7, 2017

View reviewed changes

leventov reviewed Jun 7, 2017

View reviewed changes

gianm added this to the 0.10.1 milestone Jun 8, 2017

gianm merged commit 1f2afcc into apache:master Jun 8, 2017

gianm deleted the se-expr-macros branch June 8, 2017 13:32

leventov mentioned this pull request Jun 12, 2017

Expressions work better with strings. #4394

Merged

jihoonson mentioned this pull request Jun 16, 2017

Transient failure of AppenderatorDriverTest.testPublishPerRow #4387

Closed

gianm mentioned this pull request Jul 31, 2019

Add IPv4 druid expressions #8197

Merged

5 tasks

Conversation

gianm commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Jun 5, 2017

Uh oh!

fjy commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Jun 6, 2017

Uh oh!

gianm commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Jun 7, 2017

Uh oh!

gianm commented Jun 7, 2017

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gianm commented Jun 5, 2017 •

edited

Loading

gianm Jun 7, 2017 •

edited

Loading

gianm Jun 7, 2017 •

edited

Loading