Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11520 )
Change subject: IMPALA-7543: Enhance scan ranges to support sub-ranges ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/hdfs-file-reader.cc File be/src/runtime/io/hdfs-file-reader.cc: http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/hdfs-file-reader.cc@213 PS1, Line 213: return; > Shouldn't the function return here? Yes, this was a bug. Fortunately the e2e tests also catched it. Done. http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/request-context.cc@429 PS1, Line 429: needs_buffers)); > Can you add some comments about this path? Done http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/request-context.cc@485 PS1, Line 485: AddActiveScanRangeLocked(lock, range); > Same as line 429. Done http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/scan-range.cc@210 PS1, Line 210: COUNTER_ADD_IF_NOT_NULL(reader_->bytes_read_counter_, buffer_desc->len_); : COUNTER_ADD_IF_NOT_NULL(reader_->active_read_thread_counter_, -1L); : } > Why is this 'else' branch needed? One of the last two conditions should be Done http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/scan-range.cc@263 PS1, Line 263: sub_range.offset + sub_range_pos_.bytes_read, > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/scan-range.cc@281 PS1, Line 281: K(); > This and ReadSubRanges look very similar - they could be merged to reduce c Done -- To view, visit http://gerrit.cloudera.org:8080/11520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea26ba386713990f7671aab5a372cf449b8d51e4 Gerrit-Change-Number: 11520 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 27 Sep 2018 13:10:22 +0000 Gerrit-HasComments: Yes
