Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/7822 )
Change subject: IMPALA-2494: Support for byte array encoded decimals in Parquet scanner ...................................................................... Patch Set 2: (3 comments) 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: http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-stats.inline.h@89 PS2, Line 89: switch(parquet_type){ > Do you mean to have a Decode wrapper around the templatized Decode methods? The former, so that the interface of Decode() is simpler. How this is implemented seems more a concern of the decoder than the column stats. http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h File be/src/exec/parquet-common.h: http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h@237 PS2, Line 237: ByteSize > Do you mean to have something like I think this particular call here will always return sizeof(int32_t) (line 220). I'd just put that here, since your change explicitly documents that as a template parameter. http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h@338 PS2, Line 338: template <> > thats kinda difficult because DecimalUtil::DecodeFromFixedLenByteArray is a I'm not sure I'm following: it looks like the next three methods are exactly the same. Couldn't you move them into a new method DecodeDecimalValue<T>(const uint8_t* buffer, const uint8_t* buffer_end, int fixed_len_size, T* v) and then call it and return in one line here? I may be missing something though :) -- To view, visit http://gerrit.cloudera.org:8080/7822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e Gerrit-Change-Number: 7822 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 12 Oct 2017 00:33:05 +0000 Gerrit-HasComments: Yes
