impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5307: Part 2: copy out strings in uncompressed Avro
Date Mon, 30 Oct 2017 19:33:56 GMT
Michael Ho has posted comments on this change. ( )

Change subject: IMPALA-5307: Part 2: copy out strings in uncompressed Avro

Patch Set 15:


Sorry for the delay. Some more comments. Close to completion.
File be/src/exec/
PS15, Line 213: 
If I understand it correctly, this if branch was dead code before this change, right ?
File be/src/exec/hdfs-avro-scanner.h:
PS15, Line 133: //
nit: /// to be consistent.
File be/src/exec/hdfs-scanner.h:
PS15, Line 406:   /// Codegen function to replace InitTuple(). The codegen'd version of InitTuple()
              :   /// 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.
File be/src/exec/
PS15, Line 535:       
nit: indent 4
File be/src/runtime/descriptors.h:
PS15, Line 93: if
PS15, Line 109: llvm::Constant* ToIR(LlvmCodeGen* codegen) const;
Comment: This needs to be updated should the layout of this struct change.
File be/src/runtime/tuple.h:
PS15, Line 47: /// Generate an LLVM Constant containing the offset values of this SlotOffsets
Please also comment that this function needs to be updated if the layout of this struct is
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.
File be/src/runtime/
PS15, Line 404: materialize_strings_fn
nit: Using the name copy_strings_fn will be more consistent.
PS15, Line 435: slot_offset_constants
nit: 'slot_offset_ir_constants' may make it easier to follow.
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.
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:33:56 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message