impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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. ( 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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:33:56 +0000
Gerrit-HasComments: Yes

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