Csaba Ringhofer 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 1: (3 comments) I did a quick pass, I plan to return soon. 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: } Shouldn't the function return here? 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: } else { : DCHECK(false); : } Why is this 'else' branch needed? One of the last two conditions should be always true. http://gerrit.cloudera.org:8080/#/c/11520/1/be/src/runtime/io/scan-range.cc@281 PS1, Line 281: ReadSubRangesFromCache This and ReadSubRanges look very similar - they could be merged to reduce code size. -- 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: 1 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-Comment-Date: Wed, 26 Sep 2018 22:10:25 +0000 Gerrit-HasComments: Yes
