Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/17026 )
Change subject: IMPALA-9470: Use Parquet Bloom filters - Part 1 ...................................................................... Patch Set 23: (7 comments) Hi Daniel, looks good, commented some nits. I will go through the change one more time tomorrow. http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/exec/parquet/parquet-bloom-filter-util.h File be/src/exec/parquet/parquet-bloom-filter-util.h: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/exec/parquet/parquet-bloom-filter-util.h@17 PS23, Line 17: nit: missing include guard http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/exec/parquet/parquet-bloom-filter-util.cc File be/src/exec/parquet/parquet-bloom-filter-util.cc: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/exec/parquet/parquet-bloom-filter-util.cc@18 PS23, Line 18: #include "exec/parquet/parquet-bloom-filter-util.h" : : #include <sstream> : : #include "exprs/scalar-expr-evaluator.h" : #include "util/parquet-bloom-filter.h" nit: include order, usually it is standard include directories first then user defined files http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/thirdparty/xxhash/xxhash.h File be/src/thirdparty/xxhash/xxhash.h: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/thirdparty/xxhash/xxhash.h@1 PS23, Line 1: /* Maybe we could create a Jira to disable clang on third party libraries. It looks like we would need a new directory specific clang-format file, I only did a quick research on it, so it could be more complex. http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/impala-bloom-filter-buffer-allocator.cc File be/src/util/impala-bloom-filter-buffer-allocator.cc: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/impala-bloom-filter-buffer-allocator.cc@19 PS23, Line 19: nit: empty line http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/parquet-bloom-filter-test.cc File be/src/util/parquet-bloom-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/parquet-bloom-filter-test.cc@18 PS23, Line 18: #include "util/parquet-bloom-filter.h" : #include "testutil/gtest-util.h" : : #include <cmath> : #include <unordered_set> : #include <vector> : : #include "common/names.h" nit: include order, usually it is standard include directories first then user defined files http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/parquet-bloom-filter.cc File be/src/util/parquet-bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/17026/23/be/src/util/parquet-bloom-filter.cc@18 PS23, Line 18: #include "parquet-bloom-filter.h" : : #include <cmath> : #include <cstdint> : : #include "kudu/util/slice.h" : #include "kudu/util/status.h" : #include "util/kudu-status-util.h" : : #include "thirdparty/xxhash/xxhash.h" nit: include order, usually it is standard include directories first then user defined files http://gerrit.cloudera.org:8080/#/c/17026/23/tests/query_test/test_parquet_bloom_filter.py File tests/query_test/test_parquet_bloom_filter.py: http://gerrit.cloudera.org:8080/#/c/17026/23/tests/query_test/test_parquet_bloom_filter.py@20 PS23, Line 20: nit: empty line -- To view, visit http://gerrit.cloudera.org:8080/17026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7119c7161fa3658e561fc1265430cb90079d8287 Gerrit-Change-Number: 17026 Gerrit-PatchSet: 23 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Apr 2021 14:44:50 +0000 Gerrit-HasComments: Yes
