Constant flattening in math expression by navis · Pull Request #3090 · apache/druid

navis · 2016-06-07T00:53:16Z

constant expression in math-expr can be simplified in compile time when all of the arguments are constant and the function is deterministic (currently all functions and operation in math-expr is deterministic). For example, (8 + 7) * 10) now returns simply 150 not (* (+ 8 7) 10).

nishantmonu51 · 2016-06-07T11:10:09Z

minor nit : this can be changed to single condition check,
if(eval instanceof Long)
return LongExpr(..)
else ret DoubleExpr(...)

It's problem that we don't have type information for evaluation result. Wishfully #2836 would be included in sometime. Addressed anyway, thanks.

jaehc · 2016-10-19T09:39:40Z

LGTM

dclim · 2016-11-01T20:22:19Z

needs 2 arguments, plural

fixed in #2836

dclim · 2016-11-01T20:38:48Z

especially now that you're using getFunction() to access this, making this (and the logger) private would be cleaner. Maybe add a hasFunction() if needed for ExprListenerImpl.exitFunctionExpr()

dclim · 2016-11-01T20:41:48Z

Should this really extend Function? What do you expect FunctionFactory.apply() to do?

Did this not to make two classes (factory and function) for each stateful functions. if it's look that bad, I can separate these two.

I'm okay with this for now, maybe we can revisit once you have some implementations.

dclim · 2016-11-01T21:03:08Z

Would prefer either dynamically constructing by class name or doing else if for UnaryNotExpr() and returning original expr for unimplemented cases. Someone's going to add another UnaryExpr type and this won't work right.

dclim · 2016-11-01T21:47:36Z

This would fail if the List object is immutable. Can we either enforce that the list is mutable or write into another list?

made new list

navis · 2016-11-03T01:35:47Z

rebased on #2836 and waiting it to be committed

gianm · 2016-11-03T15:13:32Z

@navis we can take another look after rebased; #2836 is committed.

navis · 2016-11-04T00:06:09Z

rebased on master

dclim · 2016-11-04T18:53:21Z

just be sure

dclim · 2016-11-04T18:54:28Z

unnecessary parentheses around value

dclim · 2016-11-04T18:55:06Z

unnecessary parentheses around value

dclim · 2016-11-04T18:56:41Z

IAE("Invalid function name '%s'", name)

dclim · 2016-11-04T18:58:16Z

Typo, fattening -> flattening I think

dclim · 2016-11-04T19:02:33Z

Minor cleanup, otherwise LGTM 👍

gianm · 2016-11-04T19:14:45Z

Why is this not instanceof ConstantExpr? It seems like there is no point to having "ConstantExpr" as an interface if we can't use it to figure out if something is a constant or not.

Is it feasible to add isConstant as a method on Expr?

I think it's nice to have all the logic for a particular expr together in the expr class, rather than spread around the project in methods like isConstant and flatten. That makes it easier to add new exprs without having to update a lot of different places in the code.

replaced with instanceof ConstantExpr

gianm · 2016-11-04T19:17:38Z

Do we expect this to happen "normally" or is this really a problem indicating a bug? If it's going to happen "normally" it shouldn't be log.info as that will be too noisy in the logs. If it indicates a bug, please bump it up to log.warn.

Bumped up to warn

gianm · 2016-11-04T19:22:53Z

I'm confused, what's going on here? It looks like this is saying that f(const) = null which doesn't seem right.

it should be Evals.isAllConstants(fattening). fixed and added test case

fjy · 2016-11-08T22:50:21Z

@navis I think there's a few remaining issues. Any chance to finish this up?

navis · 2016-11-10T00:37:59Z

@fjy I think I've addressed all comments

fjy · 2016-11-10T23:17:53Z

@nishantmonu51 any more comments?

fjy · 2016-11-14T22:14:08Z

👍

* Constant flatteing in math expression * Addressed comments and fixed some bugs * Addressed comments

navis mentioned this pull request Jun 7, 2016

Extending math expression #3021

Closed

navis force-pushed the expr-const-flatten branch from bdce500 to 94034d8 Compare June 7, 2016 02:32

navis changed the title ~~Constant flatteing in math expression~~ Constant flattening in math expression Jun 7, 2016

navis force-pushed the expr-const-flatten branch from 94034d8 to c018556 Compare June 7, 2016 07:16

nishantmonu51 reviewed Jun 7, 2016
View reviewed changes

navis force-pushed the expr-const-flatten branch from c018556 to 0ec4b1b Compare June 8, 2016 00:00

navis force-pushed the expr-const-flatten branch 4 times, most recently from bb62934 to 8f47e73 Compare October 25, 2016 23:41

dclim requested changes Nov 1, 2016

View reviewed changes

navis force-pushed the expr-const-flatten branch from 8f47e73 to 30cca12 Compare November 4, 2016 00:05

dclim reviewed Nov 4, 2016

View reviewed changes

gianm reviewed Nov 4, 2016

View reviewed changes

gianm added this to the 0.9.3 milestone Nov 4, 2016

gianm added the Improvement label Nov 4, 2016

navis force-pushed the expr-const-flatten branch from 30cca12 to 7182e0b Compare November 7, 2016 00:41

navis added 2 commits November 10, 2016 09:31

Constant flatteing in math expression

3d21d13

Addressed comments and fixed some bugs

dd0c5e3

Addressed comments

0597931

navis force-pushed the expr-const-flatten branch from 7182e0b to 0597931 Compare November 10, 2016 00:34

fjy closed this Nov 10, 2016

fjy reopened this Nov 10, 2016

dclim approved these changes Nov 10, 2016

View reviewed changes

fjy merged commit bb26636 into apache:master Nov 14, 2016

fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016

Constant flattening in math expression (apache#3090)

a9e2252

* Constant flatteing in math expression * Addressed comments and fixed some bugs * Addressed comments

dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017

Constant flattening in math expression (apache#3090)

f53dcb2

* Constant flatteing in math expression * Addressed comments and fixed some bugs * Addressed comments

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3090 Fix buffer leakage with group-by query in broker

18c0bba

Conversation

navis commented Jun 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaehc commented Oct 19, 2016

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!

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!

navis commented Nov 3, 2016

Uh oh!

gianm commented Nov 3, 2016

Uh oh!

navis commented Nov 4, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dclim commented Nov 4, 2016

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!

fjy commented Nov 8, 2016

Uh oh!

navis commented Nov 10, 2016

Uh oh!

fjy commented Nov 10, 2016