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, 23 Oct 2017 21:25:07 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 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 <tarmstrong@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, 23 Oct 2017 21:25:07 +0000
Gerrit-HasComments: Yes

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