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 15: (12 comments) Sorry for the delay. Some more comments. Close to completion. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-avro-scanner-ir.cc File be/src/exec/hdfs-avro-scanner-ir.cc: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-avro-scanner-ir.cc@a213 PS15, Line 213: If I understand it correctly, this if branch was dead code before this change, right ? http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-avro-scanner.h File be/src/exec/hdfs-avro-scanner.h: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-avro-scanner.h@133 PS15, Line 133: // nit: /// to be consistent. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-scanner.h@406 PS15, Line 406: /// Codegen function to replace InitTuple(). The codegen'd version of InitTuple() is : /// stored in 'init_tuple_fn' if codegen was successful. May help to also state the codegen'd version of the function has some constants replaced. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/exec/hdfs-scanner.cc@535 PS15, Line 535: nit: indent 4 http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/descriptors.h@93 PS15, Line 93: if is http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/descriptors.h@109 PS15, Line 109: llvm::Constant* ToIR(LlvmCodeGen* codegen) const; Comment: This needs to be updated should the layout of this struct change. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.h File be/src/runtime/tuple.h: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.h@47 PS15, Line 47: /// Generate an LLVM Constant containing the offset values of this SlotOffsets instance. Please also comment that this function needs to be updated if the layout of this struct is changed. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.h@204 PS15, Line 204: // nit: /// May help to also add that this is replacing some of the arguments passed to CopyString() in an attempt to trigger loop unrolling and constant substitution by LLVM. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.cc@404 PS15, Line 404: materialize_strings_fn nit: Using the name copy_strings_fn will be more consistent. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.cc@435 PS15, Line 435: slot_offset_constants nit: 'slot_offset_ir_constants' may make it easier to follow. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.cc@435 PS15, Line 435: Constant* Not your change but I feel it's generally less confusing to include llvm:: namespace for class not defined by codegen logic. Don't need to change it in this change. Someone can do it later in a clean-up patch. http://gerrit.cloudera.org:8080/#/c/8146/15/be/src/runtime/tuple.cc@436 PS15, Line 436: for (SlotDescriptor* slot_desc : desc.string_slots()) { : SlotOffsets offsets = {slot_desc->null_indicator_offset(), slot_desc->tuple_offset()}; : slot_offset_constants.push_back(offsets.ToIR(codegen)); : } : : Constant* constant_slot_offsets = codegen->ConstantsToGVArrayPtr( : slot_offsets_type, slot_offset_constants, "slot_offsets"); : Constant* num_string_slots = : ConstantInt::get(codegen->int_type(), desc.string_slots().size()); I think it may be helpful to add a comment on what line 435 - 444 is trying to achieve. In essence, it's converting all string slots' offsets into an array of IR constants which is a global variable named "slot_offsets". -- 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: 15 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@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, 30 Oct 2017 19:33:56 +0000 Gerrit-HasComments: Yes
