Support string type in math expression by navis · Pull Request #2836 · apache/druid

navis · 2016-04-14T00:11:01Z

Wanted to do something like __time > timestamp("2010-03-12T11:27:00) with math expr but currently it does not support that. This is the first try to support string type in math expr.

navis · 2016-05-19T00:58:40Z

rebased and refactored to be looked less awful

fjy · 2016-10-25T17:32:49Z

@himanshug can you take a look?

fjy

Took a high level look and left high level comments

fjy · 2016-10-26T00:25:24Z

as opposed to checking for type everywhere, can we use polymorphic deserialization instead? As in, we have different types of ExprEvals that are automatically instantiated based on Expr type?

I think with how this all works, it makes sense to have a type() method rather than polymorphism, since with polymorphism the value types would need to know how to operate on each other and that could get messy.

fjy · 2016-10-26T00:27:25Z

can we call this class EvaluatedExpr as opposed to ExprEval?

Can we keep this? It's trivial but makes so many conflicts in following PRs. We can discuss anytime later.

okay, not a big deal

fjy · 2016-10-26T00:29:14Z

Why can't you just return value here?

fjy · 2016-10-26T00:31:08Z

I think this class should use polymorphic deserialization to avoid so many type checks everywhere

It can also avoid passing in type everywhere

fjy · 2016-10-26T00:31:46Z

can you throw IAE instead?

fjy · 2016-10-26T00:37:02Z

escapeString?

it's generated name by antlr

fjy · 2016-10-26T00:38:56Z

why does the type have to change here?

Expression recognizes this as long type and make long type result, which is different from arithmetic post aggregator which regards all numbers as double.

Does this cause Ut failures?

navis · 2016-10-26T05:24:18Z

@fjy did you mean something like this? (f421015)

dclim · 2016-10-31T21:16:24Z

Is this needed?

It's for 'case' function which is not included in this PR. it seemed I've separated the part into another PR. I'll remove it.

dclim · 2016-10-31T21:32:20Z

Any reason we need asLong() and asDouble() in addition to longValue() and doubleValue()? These aren't currently being used. Can StringExprEval just override longValue() and doubleValue() (and probably intValue() and numericValue() as well)? This would also avoid the ClassCastException you'd get right now calling say longValue() on a StringExprEval.

Also having a method called asDouble() and one called doubleValue() is confusing to me as I'm not sure when I'd use each one, so I suggest consolidating them if possible.

dclim · 2016-10-31T21:38:53Z

If StringExprEval overrode longValue() and doubleValue() these wouldn't be necessary

I have three or four different version of cast function and this seemed the oldest one. I'll address that.

dclim · 2016-10-31T21:43:52Z

Should be able to just call x.asString() which has the same logic

yep, that is the difference of 'stringValue()' with 'asString()'. if user have checked the type of expr is X, then can use 'Xvalue()' to access the value without needing to check type again. Still would be better to remove them?

@navis IMO if there is no significant performance hit, it feels better to consolidate them as it is confusing to me which one I should use without looking through the code, but I'm open for other suggestions

dclim · 2016-10-31T21:48:37Z

Same idea, do we need stringValue() and asString()? Can we just use this logic (using String.valueOf()) in stringValue() instead of the typecast or is there performance implications?

Has some historical reason for this but ok, I'll remove some methods.

dclim · 2016-10-31T23:18:32Z

Druid favors JodaTime over java.util.Date. Any reason we can't implement this method using JodaTime?

@navis awesome

dclim · 2016-10-31T23:20:03Z

Might as well check that either 1 or 2 arguments were supplied instead of > 0

dclim · 2016-10-31T23:21:25Z

IllegalArgumentException for consistency, needs 2 arguments

dclim · 2016-10-31T23:33:57Z

I think this is different behavior from SQL NVL which doesn't treat empty strings as null. If this behavior is required, would it make sense to make an additional function to handle the nullOrEmpty case?

Some following PR will make use of dimension as input of expression(, which is the main purpose of this PR). Then it's not that clear whether empty string should be considered as null or not.

@navis I was wrong, seems that Oracle does treat empty strings as null (http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements005.htm). I'm good with this.

dclim · 2016-10-31T23:43:41Z

Would be great to see tests for the rest of the functions in Function.java if possible, or at least for the ones that were added (cast, unix_timestamp, nvl)

dclim · 2016-11-01T00:51:30Z

Also please add documentation for the new functions

3

navis · 2016-11-02T00:23:11Z

Committed addressed patch first. I think I should squash these to rebase on #3630, which is really painful.

gianm · 2016-11-02T00:45:37Z

On @dclim's "please add documentation" comment, IMO, we don't need this right now.

None of the expression stuff is documented currently. I raised #3634 for this earlier today. We need to start those docs and pay down that debt before 0.9.3, but I think it doesn't need to be in this PR since the API is still in flux. I think it makes sense to work out the API details through further PRs and then write the docs.

gianm · 2016-11-02T00:46:24Z

@navis, sorry about conflicts from #3630!

dclim · 2016-11-02T00:49:15Z

@gianm that sounds fine to me, but FYI there is a documentation page (but I don't think it's reachable from anywhere right now!): http://druid.io/docs/0.9.1.1/misc/math-expr.html

gianm · 2016-11-02T00:57:34Z

@dclim oops, I missed that. Nevermind, in that case I agree this PR should have some additions there. But let's keep it unlinked until we're happy with the API.

gianm · 2016-11-02T01:01:06Z

btw, that conflict between #3630 and here makes me wonder how we should deal with null strings in the expression language. My first instinct (not having thought about it too much) is that we should take the attitude that Druid doesn't actually generate any nulls from its columns. I'm intending to do something similar in the druid-sql I'm working on, see https://groups.google.com/d/msg/druid-development/3npt9Qxpjr0/IeJQP0YXBQAJ. i.e. if the dimension selector or an extractionFn returns null then the expression system could treat that as "".

navis · 2016-11-02T03:02:39Z

@gianm nevermind it (and it's my fault not checked related issues) and yes, null handling is really confusing and hard-to-fix issue in druid. in expression world, it's more confusing because null was not there in birth.
@dclim updated math-expr

gianm

Mostly looks good to me, just a couple of minor comments for changes, and some thoughts on null handling we can follow up in #3645

gianm · 2016-11-02T08:00:04Z

I think with how this all works, it makes sense to have a type() method rather than polymorphism, since with polymorphism the value types would need to know how to operate on each other and that could get messy.

gianm · 2016-11-02T08:05:46Z

Should this be x.type() == ExprType.STRING || y.type() == ExprType.STRING?

my bad. good catch.

gianm · 2016-11-02T08:08:10Z

It'll be very funny when someone forgets to override at least one of these 😄

gianm · 2016-11-02T08:13:08Z

Should be unix_timestamp

gianm · 2016-11-02T08:23:15Z

Hmm, I think this means all nulls, even nulls of columns we want to treat as numbers (e.g. missing columns), are getting represented as StringExprEvals. I guess that works but seems strange. It might make sense for null to have its own datatype.

I guess I don't have strong feelings on this now for this PR but we should consider null handling before finalizing the expressions API.

Raised issue in #3645 to consider this further.

dclim · 2016-11-02T21:23:27Z

@navis the point of the IAE and ISE exceptions is that it allows you to use formatted strings, so rather than doing:

throw new IAE("first argument should be string type but got " + value.type() + " type");

You can do:

throw new IAE("first argument should be string type but got %s type", value.type());

which is preferred in Druid.

dclim · 2016-11-02T21:27:17Z

stringValue() is only being used in unit tests now, if not necessary for a follow-on PR it can be removed

dclim · 2016-11-02T21:32:51Z

should just say needs 1 or 2 arguments, instead of 'at least'

dclim · 2016-11-02T23:07:01Z

I'm good with this after the last few comments and Gian's comments have been addressed.

addressed comments addressed comments Addressed comments

gianm

👍 LGTM

dclim · 2016-11-03T03:10:40Z

👍

* Support string type in math expression addressed comments addressed comments Addressed comments * Updated math function document * Addressed comments

navis mentioned this pull request Apr 14, 2016

Math expressional parameters for aggregator #2783

Merged

navis force-pushed the support-string-expr branch 2 times, most recently from efe86e9 to 7d9feab Compare April 20, 2016 11:06

navis force-pushed the support-string-expr branch from 7d9feab to 10cbe62 Compare May 19, 2016 00:58

This was referenced May 20, 2016

Ability to make derived dimension from multiple dimensions #3000

Closed

Extending math expression #3021

Closed

navis mentioned this pull request Jun 7, 2016

Constant flattening in math expression #3090

Merged

navis force-pushed the support-string-expr branch 4 times, most recently from 1fc6e6b to c2bd31d Compare October 25, 2016 00:11

fjy reviewed Oct 26, 2016

View reviewed changes

navis force-pushed the support-string-expr branch from c2bd31d to f421015 Compare October 26, 2016 05:23

dclim previously requested changes Oct 31, 2016

View reviewed changes

navis force-pushed the support-string-expr branch from f421015 to 6896510 Compare November 2, 2016 00:22

gianm added the Feature label Nov 2, 2016

gianm added this to the 0.9.3 milestone Nov 2, 2016

navis force-pushed the support-string-expr branch 2 times, most recently from 86ed3ea to c53bdcc Compare November 2, 2016 02:41

navis force-pushed the support-string-expr branch 3 times, most recently from f09cf16 to 048797e Compare November 2, 2016 02:53

navis mentioned this pull request Nov 2, 2016

Refactor boolean cast code, add tests #3016

Merged

gianm reviewed Nov 2, 2016

View reviewed changes

dclim reviewed Nov 2, 2016

View reviewed changes

navis added 2 commits November 3, 2016 08:41

Support string type in math expression

0283520

addressed comments addressed comments Addressed comments

Updated math function document

b403eb3

navis force-pushed the support-string-expr branch from 048797e to 7c93ccb Compare November 3, 2016 00:40

Addressed comments

ef40017

navis force-pushed the support-string-expr branch from 7c93ccb to ef40017 Compare November 3, 2016 01:13

gianm closed this Nov 3, 2016

gianm reopened this Nov 3, 2016

gianm approved these changes Nov 3, 2016

View reviewed changes

dclim merged commit e10def3 into apache:master Nov 3, 2016

navis mentioned this pull request Nov 4, 2016

Support virtual column for select query #2511

Merged

Conversation

navis commented Apr 14, 2016

Uh oh!

navis commented May 19, 2016

Uh oh!

fjy commented Oct 25, 2016

Uh oh!

fjy left a comment

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!

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 Oct 26, 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!

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!

gianm left a comment •

edited

Loading