Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8146 )
Change subject: IMPALA-5307: Part 2: copy out strings in uncompressed Avro ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/8146/11/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/8146/11/be/src/runtime/descriptors.cc@87 PS11, Line 87: return ConstantStruct::get(null_indicator_offset_type, : {ConstantInt::get(codegen->int_type(), byte_offset), : ConstantInt::get(codegen->tinyint_type(), bit_mask), : zeroes->getStructElement(2)}); This needs to be updated when we change the definition of NullIndicatorOffset in the future. Please add a remark about it in the header declaration. Alternately, one can consider copying the ConstantAggregateZero struct and selectively populating the fields which we deem useful. In this way, newly added field will have the zero values if we forget to initialize them but this approach may still be error-prone if we update the struct layouts in the future. Not sure if there are easy ways to add DCHECK() to check for consistency of the constant generated. http://gerrit.cloudera.org:8080/#/c/8146/11/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: http://gerrit.cloudera.org:8080/#/c/8146/11/be/src/runtime/tuple.cc@397 PS11, Line 397: Status( Status::Expected() ? http://gerrit.cloudera.org:8080/#/c/8146/11/be/src/runtime/tuple.cc@449 PS11, Line 449: Value* result_val = builder.CreateCall(cross_compiled_fn, Just curious if you inspected the output IR to see if it's actually unrolling the loop ? -- To view, visit http://gerrit.cloudera.org:8080/8146 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1fc78790d778c874f5aafa5958c3c045a88d233 Gerrit-Change-Number: 8146 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Oct 2017 21:25:07 +0000 Gerrit-HasComments: Yes
