Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8523 )
Change subject: IMPALA-5931: Generates scan ranges in planner for s3/adls ...................................................................... Patch Set 7: Code-Review+1 (7 comments) Mostly minor comments but I am happy with the FE changes. You should ask someone to take another look at BE as well. http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/scheduling/scheduler.cc@509 PS7, Line 509: expanded_locations Maybe rename to 'generated_scan_ranges'? http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/scheduling/scheduler.cc@509 PS7, Line 509: vector<TScanRangeLocationList>* expanded_locations, : int* num_generated) { Indentation is off http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/util/compress.h File be/src/util/compress.h: http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/util/compress.h@136 PS7, Line 136: flat buffer nit: FlatBuffer so that it is clear what this thing represents http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/util/compress.h@139 PS7, Line 139: nit: indentation http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/util/compress.cc File be/src/util/compress.cc: http://gerrit.cloudera.org:8080/#/c/8523/7/be/src/util/compress.cc@325 PS7, Line 325: THdfsCompression::type* thrift_compression) { Your indentation is off is several places http://gerrit.cloudera.org:8080/#/c/8523/7/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/8523/7/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@154 PS7, Line 154: nit: extra space http://gerrit.cloudera.org:8080/#/c/8523/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8523/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@738 PS7, Line 738: supportsBlocks in the other file you're using hasBlockInfo. I think I prefer this one better :) -- To view, visit http://gerrit.cloudera.org:8080/8523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I326065adbb2f7e632814113aae85cb51ca4779a5 Gerrit-Change-Number: 8523 Gerrit-PatchSet: 7 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 22 Jan 2018 19:50:13 +0000 Gerrit-HasComments: Yes
