Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13507 )
Change subject: IMPALA-8450: Add support for zstd in parquet ...................................................................... Patch Set 4: (4 comments) Minor comments only then I can +2 http://gerrit.cloudera.org:8080/#/c/13507/4/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/13507/4/be/src/service/query-options-test.cc@23 PS4, Line 23: #include <gutil/strings/substitute.h> This should be surrounded by double quotes and in the lower block, since gutil is just part of our codebase at this point. http://gerrit.cloudera.org:8080/#/c/13507/4/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/13507/4/be/src/service/query-options.cc@283 PS4, Line 283: if (tokens.size() > 2) { return Status("Invalid compression codec value"); } Remove braces for one line if http://gerrit.cloudera.org:8080/#/c/13507/4/be/src/service/query-options.cc@285 PS4, Line 285: string codec_name = tokens[0]; nit: could be const string& http://gerrit.cloudera.org:8080/#/c/13507/4/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/13507/4/tests/query_test/test_insert_parquet.py@150 PS4, Line 150: base_table = "%s.%s" % (unique_database, "t1_default") nit: prefer "{0}.{1}".format(...) - it's considered a somewhat better practice than the % operator. -- To view, visit http://gerrit.cloudera.org:8080/13507 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id2c0e26e6f7fb2dc4024309d733983ba5197beb7 Gerrit-Change-Number: 13507 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 04 Jun 2019 20:18:54 +0000 Gerrit-HasComments: Yes
