impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
Date Wed, 08 Feb 2017 16:17:06 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

Patch Set 19:

File fe/src/main/java/org/apache/impala/analysis/

Line 57:   private final ArrayList<Type> aggFnArgTypes_;
> Is this change a nice add-on or is it required for this patch?
The UDA codegen and FunctionContext::Get*Type() semantic fixes are somewhat independent but
touch the same code so it was easiest to combine them. This is needed for GetArgType() to
return the correct input expr time.

We discussed the approach offline. It's the most sane way to plumb it through (otherwise the
merge aggregation has to go and search through the plan tree for the original AggregateInfo
when it is generating the thrift descriptors, which would violate multiple abstractions).

Alex found an example where the types do indeed get out of sync:

  select typeof(sum(f + d)) from (select float_col f, 1.2 d from functional.alltypes) v;

However, in this case (and all cases I can think of) , the intermediate and output tuple descriptors
are also inconsistent, which is detected by a precondition:

  ERROR: IllegalStateException: Agg expr sum(float_col + 1.2) returns type DOUBLE but its
output tuple slot has type DECIMAL(38,9)

I added an additional precondition to check for the types getting out of sync. I verified
that it's hit if I comment out the earlier preconditions:

  ERROR: IllegalStateException: Agg expr sum:merge(f + d) arg type DECIMAL(38,9) differs from
input expr type DOUBLE in original expr sum(float_col + 1.2)

So we need to fix the underlying bug still (which actually stems from the decimal type resolution
algorithm) but the behaviour won't be a regression.

Line 74:   private FunctionCallExpr(FunctionName fnName, FunctionParams params,
> don't need this anymore, right?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message