Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/15051 )
Change subject: IMPALA-9226: Improve string allocations of the ORC scanner ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.h File be/src/exec/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.h@207 PS2, Line 207: /*&& !orc_batch->isEncoded*/ nit: pls remove commented code http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.cc@150 PS2, Line 150: if (blob_ == nullptr) { nit: It is probably just personal preference but I find it more readable to return early in edge cases. This saves some indentation later on. In this case if (blob_ != nullptr) return Status::OK() http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.cc@151 PS2, Line 151: TODO Is this TODO going to be taken care within this patch or do you expect this to be handled somewhere in the future unrelated to this change? If the second one then opening a Jira and adding the Jira ID to the comment would make sense. (I see that the TODO was added after a review comment requested it, though). http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.cc@167 PS2, Line 167: nit : too many spaces here and below http://gerrit.cloudera.org:8080/#/c/15051/2/be/src/exec/orc-column-readers.cc@176 PS2, Line 176: InitBlob I'm wondering if calling InitBlob() could move into UpdateInputBatch() from here and from L186. This way you don't have to do the "if (blob_ == nullptr)" check in every ReadValue() through the InitBlob() functions just simply you can DCHECK that it's not null. -- To view, visit http://gerrit.cloudera.org:8080/15051 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If2d975946fb6f4104d8dc98895285b3a0c6bef7f Gerrit-Change-Number: 15051 Gerrit-PatchSet: 2 Gerrit-Owner: Norbert Luksa <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 20 Jan 2020 14:19:42 +0000 Gerrit-HasComments: Yes
