Tim Armstrong has posted comments on this change.

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

Patch Set 2:


File be/src/exec/partitioned-aggregation-node.cc:

Line 1064:     // To minimize branching on the UpdateTuple path, initialize the 
result value
> The details of how this saves branches was somewhat nebulous to me. Seems w
Added an example that hopefully makes it clearer.
Also fix it to refer to the Add() operation, not Update()

Line 1597:     Value* expr_ctx_ptr = builder.CreateGEP(
> inbounds?
Yep, good idea. I just fixed it everywhere.

Line 1658:     // properties. First, for these builtins, null input values can 
be skipped. Second,

Line 1747:   // Create pointers to input args to pass to uda_fn. We must use 
the unlowered type.
Let me know if I can help explain any of this. The stuff with lowered and 
unlowered values is annoyingly convoluted but stems from some limitations of 
CodegenAnyVal. I had a draft patch to clean it up but it's a pretty big change 
and doesn't have much immediate impact.

File be/src/exec/partitioned-aggregation-node.h:

Line 654:   void CodegenBranchIfNull(LlvmCodeGen* codegen, LlvmBuilder* builder,
> Does it make sense to put this into LlvmCodegen? Seems generally useful.
Done. Moved it to CodegenAnyVal.

File be/src/exprs/aggregate-functions-ir.cc:

Line 630: static inline StringVal ALWAYS_INLINE DefaultStringConcatDelim() {
> typo: not

File be/src/runtime/descriptors.cc:

Line 580:       builder->CreateGEP(tuple_bytes, null_byte_offset, 
> inbounds? (and elsewhere)

Line 619:       result = builder->CreateOr(null_byte, mask, "null_bit_cleared");
> null_bit_set

Line 624:   } else {
> Why would we not require is_null to be a ConstantInt? Do we exercise the co
Yes, I added it for use in CodegenUpdateSlot():

  Value* result_is_null = result_val.GetIsNull("result_is_null");
          codegen, &builder, agg_tuple_arg, result_is_null);

File be/src/runtime/descriptors.h:

Line 139:   /// represented as a 1-bit integer indicating whether this slot is 
null in 'tuple'.
> 1-byte integer?
Nope, it's a 1-bit integer (LLVM has an internal logical type that can get 
mapped in various ways onto CPU flag registers or wider integers). Called it 
llvm i1 so it's more obvious that it's that.

Line 144:   /// NULL bit in the given 'tuple' to the 'value' in null. 'tuple' 
is a pointer
> typo: value 'is_null'

Line 145:   /// to the tuple, and 'null' is an boolean value represented as a 
1-bit integer.
> some typos in this sentence, should be 'is_null'

File tests/query_test/test_aggregation.py:

Line 31: agg_functions = ['sum', 'count', 'min', 'max', 'avg']
> add ndv if easy enough? alternatively file a JIRA for adding more coverage 

Line 33: data_types = ['int', 'bool', 'double', 'bigint', 'tinyint',
> maybe you can squeeze string into this patch if it's easy enough

I think we have decent decimal coverage from decimal.test at least.

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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to