Tim Armstrong 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 4: (3 comments) Few more comments related to previous ones. http://gerrit.cloudera.org:8080/#/c/7822/4/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/7822/4/be/src/util/dict-encoding.h@104 PS4, Line 104: PHYSICAL_TYPE Isn't this PHYSICAL_TYPE the internal type, which is called LOGICAL_TYPE in the decoder below? http://gerrit.cloudera.org:8080/#/c/7822/4/be/src/util/dict-encoding.h@199 PS4, Line 199: PHYSICAL_TYPE It doesn't seem like the whole decoding class needs to be templated by this parameter, since it only affects the behaviour of the Reset() function that actually reads the dictionary. http://gerrit.cloudera.org:8080/#/c/7822/4/be/src/util/dict-encoding.h@323 PS4, Line 323: DictDecoder<Decimal16Value, parquet::Type::BYTE_ARRAY>::GetNextValue( The logic here is also the same as above - we should find a way to avoid the duplication. -- 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: 4 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: Mon, 16 Oct 2017 22:13:18 +0000 Gerrit-HasComments: Yes
