Math expressional parameters for aggregator

[navis]

Someone wanted druid to support expressional parameter for aggregator something like sum(a + b / c). I don't know why it's needed either but this is it. Will make propose in dev mailing list soon after.

Based on great work(#2090) of @himanshug.

[julianhyde]

Do you allow quoting of column names? Are column names case-sensitive? I suggest you make the syntax as close to SQL as possible. That would also mean supporting quoting, and also using 'iif' rather than 'if'.

[navis]

@julianhyde Currently it does not allow quoting and identifiers are case-sensitive. I've used double quote for string literal in #2836 but it would be better to change it single quote. I'll handle your comments in #2836.

[julianhyde]

@navis Thanks! We know that this expression language is going to be popular, and then we're going to wish that it as close to SQL as we can manage, otherwise we'll spend years trying to undo the differences.

[xvrl]

@julianhyde we made the decision back in Druid 0.7 to have column names be case-sensitive to keep things simple implementation-wise. As far as I know SQL doesn't prescribe things to be case-insensitive for column names. For instance PostgreSQL is case-sensitive, but has it's SQL layer fold column names to lower-case, if not quoted. I think it would be worthwhile to have a similar behavior for Druid where any case-folding is left to the SQL layer doing the query translation.

[navis]

@xvrl didn't changed case-sensitivity of identifiers and have no intention to do that, but possibly make an option for it.

[julianhyde]

It's totally valid and workable to have case-sensitive identifiers and leave their case intact (i.e. not convert them to lower case as PostgreSQL does, or to upper case as Oracle does).

The only other thing is spaces in column names. If you allow space (and other punctuation) in column names then you need a way to use these in expressions. You might use double quotes (as Oracle and PostgreSQL), brackets (as SQL Server) or back-quotes (as MySQL).

[navis]

I think this is duplicate of #1965. @himanshug could you review this?

[himanshug]

@navis yes, i planned to add expression language to aggregators and post-aggregators in #1965 , was waiting for expression thingie to get merged.
i see you have multiple PRs in flight on same , i will review these as soon as i can.

[navis]

Squashed and merged with #2820, caused by hard-maintenance for long abandoned time.

[gianm]

@navis, thanks for the patch, looks great modulo some comments.

@himanshug, you started the original math expression, are you interested in taking a look at this patch? This would also be the first time the math expression stuff is actually exposed as an external API - do you have any concerns?

We will need documentation for expressions at some point, but I think it's ok to merge this without that, and leave it as an undocumented feature for now in master. Ideally before 0.9.3 we will write up some docs, firm up the API, and make this a documented feature.

[gianm]

IMO this would be nicer as a method on Expr like List<String> requiredBindings(). instanceof generally feels brittle and better to avoid if possible.

[himanshug]

+1

[navis]

replaced with visitor, which seemed more elegant

[gianm]

Suggest naming this objectBindings

[navis]

ok. no one will use druid with javafx.

[gianm]

Suggest naming this supplierBindings

[gianm]

Suggest calling this "expression" to be consistent with the aggregators' "fieldExpression"

[navis]

sure

[gianm]

Would be nice to verify here that exactly one of fieldName / fieldExpression is set, but not both.

[navis]

done

[gianm]

great method :)

[navis]

thanks.

[gianm]

else throw exception?

[navis]

Yep, there was double type here and looks like I removed them altogether by mistake. This would be rewritten with #2836, as commented above.

[gianm]

It looks like the intent is missing/mistyped columns will be bound as null. Does the math expression library handle that well? what happens exactly?

[navis]

It'll throw NPE in evaluation. MathExpressionSelector should be rewritten with following PR #2836, anyway

[navis]

With that, null is handled as null string and translated to 0 whenever it's needed to be numeric type.

[himanshug]

null -> druid-defaults (empty string or 0) sounds good to me

[gianm]

throw UnsupportedOperationException preferred to returning null IMO.

[navis]

All others returns null. It's named DUMMY_COLUMN_SELECTOR_FACTORY

[gianm]

this could take Expr to avoid parsing overhead for each segment's query runner.

[himanshug]

did a quick scan, overall looks fine to me... can give a more careful look next week.
i'll be happy to see the expressions in to action finally.

[erikdubbelboer]

Will there also be some documentation updates for this?

[gianm]

@erikdubbelboer yes, although likely in a future PR (the expression stuff this is based on was never documented).

[gianm]

👍 LGTM after rebase to address conflicts from #3551.

[jon-wei]

How come this NumericColumnSelector reads everything as floats? Should there be a check for long type as well?

[jon-wei]

similar comment, should this check for long type and call getLongMetric() in that case?

[gianm]

similar suggestion to above could work here too.

[himanshug]

agree, that we should be supporting long in all the places

[jon-wei]

Can you elaborate on the choice for this ordering?

Should infinite > numeric? That seems more natural to me

[gianm]

The default is Long.compare / Double.compare, this code only gets used if you say "ordering" : "numericFirst". So I think that's okay.

[gianm]

Hmm, this could probably do something like,

Object val = row.getRaw(columnName);
return val instanceof Number ? val : row.get().getFloatMetric(columnName);

Most of the time, val should be an instanceof number. The getFloatMetric should only trigger if it's a string we're trying to cast to a number.

[navis]

@jon-wei @gianm Yep, you guys can find similar code in IncrementalIndex, too. Actually it's fixed in #2836 (of our branch) but I'll address that in here.

[gianm]

Still 👍 on this, although consider https://github.com/druid-io/druid/pull/2783/files#r83581162.

[himanshug]

not sure why we can't keep these private anymore?

[navis]

We need to access fields when traversing the expression.

[himanshug]

static here is probably not needed as enums are static classes anyways

[navis]

It's copy from ArithmeticPostAggregator$Ordering. Would it be better to separate this into independent class?

[gianm]

Ah ha, I knew I saw this somewhere before :)

Yeah, I think we could have a set of NumberComparators utilities sort of like we have StringComparators. Separate PR would be fine by me though.

[himanshug]

these two are repeated everywhere, is it appropriate to change method signature to take arguments (Expression expr, List requiredBindings) ?

[navis]

I think it's not necessary.

[gianm]

👍 to changes, thx @navis.

[jon-wei]

new changes LGTM, 👍

[gianm]

thx @jon-wei for taking a look. @himanshug, any further comments?