Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18815 )
Change subject: IMPALA-10800: Tidy up the be/src/exec directory ...................................................................... Patch Set 2: (39 comments) Nice patch, thanks! http://gerrit.cloudera.org:8080/#/c/18815/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18815/2//COMMIT_MSG@9 PS2, Line 9: makes the Nit: these words are not needed. http://gerrit.cloudera.org:8080/#/c/18815/2//COMMIT_MSG@10 PS2, Line 10: Nit: Unnecessary space. http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner-ir.cc File be/src/exec/avro/hdfs-avro-scanner-ir.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner-ir.cc@22 PS2, Line 22: #include "hdfs-avro-scanner.h" This should be included as "exec/avro/hdfs-avro-scanner.h", as in https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner-test.cc File be/src/exec/avro/hdfs-avro-scanner-test.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner-test.cc@18 PS2, Line 18: #include "hdfs-avro-scanner.h" This should be included as "exec/avro/hdfs-avro-scanner.h", as in https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner.cc File be/src/exec/avro/hdfs-avro-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/avro/hdfs-avro-scanner.cc@18 PS2, Line 18: #include "hdfs-avro-scanner.h" This should be included as "exec/avro/hdfs-avro-scanner.h", as in https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-scan-node.h File be/src/exec/hbase/hbase-scan-node.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-scan-node.h@24 PS2, Line 24: #include "hbase-table-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-scan-node.cc File be/src/exec/hbase/hbase-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-scan-node.cc@18 PS2, Line 18: #include "hbase-scan-node.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-scanner.cc File be/src/exec/hbase/hbase-table-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-scanner.cc@18 PS2, Line 18: #include "hbase-table-scanner.h" : #include "hbase-scan-node.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-sink.h File be/src/exec/hbase/hbase-table-sink.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-sink.h@28 PS2, Line 28: #include "hbase-table-writer.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-sink.cc File be/src/exec/hbase/hbase-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-sink.cc@18 PS2, Line 18: #include "hbase-table-sink.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-writer.cc File be/src/exec/hbase/hbase-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hbase/hbase-table-writer.cc@18 PS2, Line 18: #include "hbase-table-writer.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/hdfs-scan-node-base.cc@21 PS2, Line 21: #include "exec/hdfs-columnar-scanner.h" Here I think the order should be first top-level files in exec, then exec subfolders, all in alphabetical order. For example 'avro' should be the first subfolder. http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-base.cc File be/src/exec/kudu/kudu-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-base.cc@18 PS2, Line 18: #include "kudu-scan-node-base.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-base.cc@27 PS2, Line 27: #include "kudu-scanner.h" : #include "kudu-util.h" These should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-mt.h File be/src/exec/kudu/kudu-scan-node-mt.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-mt.h@21 PS2, Line 21: #include "kudu-scan-node-base.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-mt.cc File be/src/exec/kudu/kudu-scan-node-mt.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-mt.cc@18 PS2, Line 18: #include "kudu-scan-node-mt.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node-mt.cc@23 PS2, Line 23: #include "kudu-scanner.h" : #include "kudu-util.h" These should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node.h File be/src/exec/kudu/kudu-scan-node.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node.h@25 PS2, Line 25: #include "kudu-scan-node-base.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node.cc File be/src/exec/kudu/kudu-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node.cc@18 PS2, Line 18: #include "kudu-scan-node.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scan-node.cc@23 PS2, Line 23: #include "kudu-scanner.h" : #include "kudu-util.h" These should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scanner.h File be/src/exec/kudu/kudu-scanner.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scanner.h@25 PS2, Line 25: #include "kudu-scan-node-base.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scanner.cc File be/src/exec/kudu/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scanner.cc@18 PS2, Line 18: #include "kudu-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-scanner.cc@27 PS2, Line 27: #include "kudu-util.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-table-sink.h File be/src/exec/kudu/kudu-table-sink.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-table-sink.h@26 PS2, Line 26: #include "kudu-util.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-table-sink.cc File be/src/exec/kudu/kudu-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-table-sink.cc@18 PS2, Line 18: #include "kudu-table-sink.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-table-sink.cc@28 PS2, Line 28: #include "kudu-util.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-util-ir.cc File be/src/exec/kudu/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-util-ir.cc@18 PS2, Line 18: #include "kudu-util.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-util.cc File be/src/exec/kudu/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/kudu/kudu-util.cc@18 PS2, Line 18: #include "kudu-util.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/CMakeLists.txt File be/src/exec/orc/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/CMakeLists.txt@26 PS2, Line 26: ORC Nit: could be "Orc". http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/hdfs-orc-scanner.h File be/src/exec/orc/hdfs-orc-scanner.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/hdfs-orc-scanner.h@32 PS2, Line 32: #include "orc-metadata-utils.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/hdfs-orc-scanner.cc File be/src/exec/orc/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/hdfs-orc-scanner.cc@18 PS2, Line 18: #include "hdfs-orc-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/hdfs-orc-scanner.cc@24 PS2, Line 24: #include "orc-column-readers.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-column-readers.h File be/src/exec/orc/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-column-readers.h@24 PS2, Line 24: #include "hdfs-orc-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-column-readers.cc File be/src/exec/orc/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-column-readers.cc@18 PS2, Line 18: #include "orc-column-readers.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-metadata-utils.cc File be/src/exec/orc/orc-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/orc/orc-metadata-utils.cc@18 PS2, Line 18: #include "orc-metadata-utils.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/rcfile/CMakeLists.txt File be/src/exec/rcfile/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/rcfile/CMakeLists.txt@26 PS2, Line 26: RCFile Nit: I would use "Rcfile" but this is good too. http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/rcfile/hdfs-rcfile-scanner.cc File be/src/exec/rcfile/hdfs-rcfile-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/rcfile/hdfs-rcfile-scanner.cc@18 PS2, Line 18: #include "hdfs-rcfile-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/sequence/hdfs-sequence-scanner.cc File be/src/exec/sequence/hdfs-sequence-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/sequence/hdfs-sequence-scanner.cc@18 PS2, Line 18: #include "hdfs-sequence-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/text/hdfs-plugin-text-scanner.cc File be/src/exec/text/hdfs-plugin-text-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18815/2/be/src/exec/text/hdfs-plugin-text-scanner.cc@18 PS2, Line 18: #include "hdfs-plugin-text-scanner.h" This should be included using the whole path like https://github.com/apache/impala/blob/3ed71756c4e6f575d73133ff8ce891bfebaff4e4/be/src/exec/parquet/hdfs-parquet-scanner.cc#L18 -- To view, visit http://gerrit.cloudera.org:8080/18815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie936c400ea8b112073bba892497ab8a1498c418d Gerrit-Change-Number: 18815 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 10 Aug 2022 13:28:33 +0000 Gerrit-HasComments: Yes
