impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions
Date Tue, 08 Nov 2016 01:15:38 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions
......................................................................


Patch Set 12:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

Line 103:       DCHECK(false) << "NYI:" << type.DebugString();
> why do we not need these still with the removal of the bail outs for CHAR?
The char bailout was still there, but more implicit.


Line 462: void CodegenAnyVal::SetFromRawPtr(Value* raw_ptr) {
> this looks unused. remove?
Done


http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:

Line 53: /// TYPE_TIMESTAMP/TimestampVal: { i64, i64 }
> decimal?
Done


http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

Line 1071:     // eliminate a branch per value.
> what code is this paragraph referring to? should it precede the Init() call
It probably makes sense to move it up, since it's implemented by both Init() and the below
code.


Line 1079:         && evaluator->intermediate_type().type != TYPE_TIMESTAMP) {
> IsTimestampType() for consistency now that most types have that?
Done


PS12, Line 1454: :
> maybe explain that we hand-generate a code sequence in this case.
Done


PS12, Line 1670: .type != TYPE_TIMESTAMP
> IsTimestampType()?
Done


PS12, Line 1742: We must use the unlowered type
> would be good to briefly explain why rather than just the what.
Done


Line 1811:                   "intermediate tuple desc");
> when would this happen?
If there's a char field in the tuple. I think this is a step back in terms of error messages,
so I added back an explicit check for CHAR fields in the intermediate tuple.

I actually noticed there are a couple of places in the codebase that don't check for a null
result from this function, so I fixed those.


http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS12, Line 630: ;
> remove
Done


http://gerrit.cloudera.org:8080/#/c/4655/12/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

PS12, Line 143: the
> "a boolean value represented ..." ? like above comment.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id9dc21d1d676505d3617e1e4f37557397c4fb260
Gerrit-PatchSet: 12
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: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message