Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16893 )
Change subject: IMPALA-6434: Add support to decode RLE_DICTIONARY encoded pages ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/16893/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16893/3//COMMIT_MSG@10 PS3, Line 10: PLAIN/ > PLAIN is the new way AFAIK, so we use PLAIN for the dictionary page and RLE Done. Good point http://gerrit.cloudera.org:8080/#/c/16893/3//COMMIT_MSG@10 PS3, Line 10: old PLAIN/PLAIN_DICTIONARY values. > Maybe you could emphasise that the data is still encoded the same way. Done http://gerrit.cloudera.org:8080/#/c/16893/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc File be/src/exec/parquet/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/16893/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc@92 PS3, Line 92: use > nit: maybe write_new_parquet_dictionary_encodings to be more explicit? Done http://gerrit.cloudera.org:8080/#/c/16893/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc@881 PS3, Line 881: current_encoding_ > I wonder if the code would be cleaner/less error-prone if 'current_encoding I made the switch to doing this. I'd initially thought that it would be bad to add the extra branch in ProcessValue() but on further thought it doesn't really make sense that it would matter, it should be a predictable branch. http://gerrit.cloudera.org:8080/#/c/16893/3/be/src/exec/parquet/parquet-column-readers.cc File be/src/exec/parquet/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/16893/3/be/src/exec/parquet/parquet-column-readers.cc@326 PS3, Line 326: so > nit: to Done http://gerrit.cloudera.org:8080/#/c/16893/3/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/16893/3/testdata/data/README@593 PS3, Line 593: > is the newline intentional? Done -- To view, visit http://gerrit.cloudera.org:8080/16893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I90942022edcd5d96c720a1bde53879e50394660a Gerrit-Change-Number: 16893 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <tarmstr...@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-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Jan 2021 07:57:34 +0000 Gerrit-HasComments: Yes
