Bikramjeet Vig has posted comments on this change. Change subject: Rough draft for IMPALA-5628 ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 1254: // where to put that check? refer dcheck added below > This could possibly go with the other metadata checks, after initializing t makes sense, will do http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-stats.inline.h File be/src/exec/parquet-column-stats.inline.h: Line 89: switch(parquet_type){ > This switch on the parquet type looks like it may fit better in the Parquet Do you mean to have a Decode wrapper around the templatized Decode methods? or just keep the current single templated Decode and switch on parquet_type inside each specialized Decode? http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h File be/src/exec/parquet-common.h: PS2, Line 237: ByteSize > These calls could now be removed and replaced with the bytesize derived fro Do you mean to have something like - template <parquet::Type::type PARQUET_TYPE> int ParquetPlainEncoder::ByteSize() , and then use overloading to select which type is passed, not sure how much perf impact will there be for overloading here OR - template <typename T, parquet::Type::type PARQUET_TYPE> int ParquetPlainEncoder::ByteSize() Line 338: template <> > Can this be deduplicated? thats kinda difficult because DecimalUtil::DecodeFromFixedLenByteArray is also templated and extracting the code before that into a common inlined function is not worth it because we need the 3 variables defined there in our call to DecodeFromFixedLenByteArray and to return, so would have pass 3 pointers to that method (and probably call it init or something like that) which might make the code ugly -- To view, visit http://gerrit.cloudera.org:8080/7822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
