Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11949 )
Change subject: IMPALA-7869: break up parquet-column-readers.cc ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h File be/src/exec/parquet-bool-decoder.h: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-bool-decoder.h@17 PS7, Line 17: > We have several parquet_* files in exec/ - it would be nice to move this to Yeah I thought about this too - the number of files under exec/ has gotten a bit unmanagable. Anyway I just went ahead and did it. Git does have some ability to detect moves and handle the rebase automatically so hopefully it won't be too disruptive. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@74 PS7, Line 74: i > Where is this new variable used? It's used in an macro defined in the header. Added comment. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-column-readers.cc@284 PS7, Line 284: /// The size of this column with plain encoding for FIXED_LEN_BYTE_ARRAY, or : /// the max length for VARCHAR columns. Unused otherwise. : int fixed_len_size_; : : /// Contains extra data needed for Timestamp decoding. : ParquetTimestampDecoder timestamp_decoder_; : : /// Contains extra state required to decode boolean values. Only initialised for : /// BOOLEAN columns. : unique_ptr<ParquetBoolDecoder> bool_decoder_; > A note about members that are used only in specific types: I have a plan to Yeah I think using a templated implementation makes sense. http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h File be/src/exec/parquet-level-decoder.h: http://gerrit.cloudera.org:8080/#/c/11949/7/be/src/exec/parquet-level-decoder.h@122 PS7, Line 122: /// The parquet encoding used for the levels. Only RLE is supported for now. : parquet::Encoding::type encoding_ = parquet::Encoding::PLAIN; > This was not changed, but looks a bit weird (only RLE is supported, that it That's a good point. Removed encoding_ and updated comments. -- To view, visit http://gerrit.cloudera.org:8080/11949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efd5c50b781fe9e3c022b33c66c06cfb529c0b8 Gerrit-Change-Number: 11949 Gerrit-PatchSet: 7 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-Comment-Date: Tue, 20 Nov 2018 17:41:59 +0000 Gerrit-HasComments: Yes
