impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2494: Support for byte array encoded decimals in Parquet scanner
Date Mon, 16 Oct 2017 22:13:18 GMT
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.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Oct 2017 22:13:18 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message