Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/13396 )
Change subject: IMPALA-8450: Add support for zstd and lz4 in parquet ...................................................................... Patch Set 7: (56 comments) Thanks for the review comments. http://gerrit.cloudera.org:8080/#/c/13396/6/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/13396/6/be/src/service/query-options.cc@761 PS6, Line 761: > nit: could be moved to the previous line Done http://gerrit.cloudera.org:8080/#/c/13396/6/be/src/util/codec.h File be/src/util/codec.h: http://gerrit.cloudera.org:8080/#/c/13396/6/be/src/util/codec.h@63 PS6, Line 63: THdfsCompression::type format_; : // Currently only ZSTD uses compression level. : int compression_level_; : }; > I would prefer to make the members public. If you want to keep private memb Made the members public. http://gerrit.cloudera.org:8080/#/c/13396/6/be/src/util/codec.h@68 PS6, Line 68: eate a > Can you mention in a comment that currently only ZSTD uses this? Done http://gerrit.cloudera.org:8080/#/c/13396/6/cmake_modules/FindZstd.cmake File cmake_modules/FindZstd.cmake: http://gerrit.cloudera.org:8080/#/c/13396/6/cmake_modules/FindZstd.cmake@36 PS6, Line 36: > nit: whitespace consistency Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@127 PS5, Line 127: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@144 PS5, Line 144: > flake8: E115 expected an indented block (comment) Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@179 PS5, Line 179: > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@181 PS5, Line 181: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@181 PS5, Line 181: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@182 PS5, Line 182: t > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@182 PS5, Line 182: . > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@182 PS5, Line 182: _ > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@182 PS5, Line 182: l > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@183 PS5, Line 183: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@183 PS5, Line 183: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@183 PS5, Line 183: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@183 PS5, Line 183: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@184 PS5, Line 184: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@184 PS5, Line 184: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@185 PS5, Line 185: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@185 PS5, Line 185: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@186 PS5, Line 186: s > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@186 PS5, Line 186: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@186 PS5, Line 186: T > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@187 PS5, Line 187: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@188 PS5, Line 188: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@188 PS5, Line 188: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@189 PS5, Line 189: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@189 PS5, Line 189: t > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@189 PS5, Line 189: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@191 PS5, Line 191: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@191 PS5, Line 191: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@191 PS5, Line 191: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@191 PS5, Line 191: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@192 PS5, Line 192: m > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@192 PS5, Line 192: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@193 PS5, Line 193: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@193 PS5, Line 193: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@194 PS5, Line 194: r > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@194 PS5, Line 194: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@194 PS5, Line 194: d > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@194 PS5, Line 194: l > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@195 PS5, Line 195: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@195 PS5, Line 195: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@196 PS5, Line 196: r > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@196 PS5, Line 196: t > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@196 PS5, Line 196: I > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@197 PS5, Line 197: c > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@198 PS5, Line 198: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@198 PS5, Line 198: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@198 PS5, Line 198: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@198 PS5, Line 198: > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@199 PS5, Line 199: e > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/5/tests/query_test/test_insert_parquet.py@199 PS5, Line 199: I > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/13396/6/tests/query_test/test_insert_parquet.py File tests/query_test/test_insert_parquet.py: http://gerrit.cloudera.org:8080/#/c/13396/6/tests/query_test/test_insert_parquet.py@144 PS6, Line 144: > I would prefer to move most of the logic to a .test file. Moved the table setup logic to insert_parquet_multi_codecs.test file. http://gerrit.cloudera.org:8080/#/c/13396/6/tests/query_test/test_insert_parquet.py@159 PS6, Line 159: : @classmethod : def get_workload(self): : return 'functional-query' : : @classmethod : def add_test_dimensions(cls): > The test could be probably shorter by CTAS-ing/inserting from alltypestiny, I ended up using insert from sub select. Was able to include types not in functional.alltypestiny using casts. Added a couple rows with nulls. The test case now populates t1_default using default codec and populates t1_zstd_lz4 using lz4/zstd. Finally, we compare the data from the two tables. -- To view, visit http://gerrit.cloudera.org:8080/13396 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98c6dcf3d0a873380e4fa4cf03eb7e924e4ee768 Gerrit-Change-Number: 13396 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 22 May 2019 23:00:12 +0000 Gerrit-HasComments: Yes
