impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
Date Wed, 08 Feb 2017 22:15:00 GMT
Alex Behm has posted comments on this change.

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


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5161/19/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 55:   // If this is merge aggregation function, the input argument types of the aggregate
> Doesn't that somehow entangle the lifecycles of the two different sets of e
I'm fine with either approach, but:

This comment should explain precisely what is inside aggFnArgTypes_. Even the variable name
is imprecise because these are not "function argument types" since those could be derived
from the function signature.

That's why I'm saying that having the cloned parent expr is clearer - because we get the types
from the children of an actual expr instance and not the function signature.


-- 
To view, visit http://gerrit.cloudera.org:8080/5161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message