Tim Armstrong has posted comments on this change. Change subject: IMPALA-2494: Support for byte array-encoded decimals in Parquet scanner ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/5115/3/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: Line 1126: switch (slot_desc->type().GetByteSize()) { I think the code might work out simpler if we added parquet::Type as a template parameter to ScalarColumnReader and Encode/Decode. I think that keeps things more consistent since then type parameters are always handled in the same way (rather than handling this one case with a runtime check). I think it makes sense conceptually to have both the source parquet type and destination Impala types be template parameters (it's mostly coincidental that each Impala type mapped to a single Parquet type up until now). It seems like the right framework for future changes of this nature where we want to have multiple parquet types mapping to an Impala type - instead of having to convince ourselves that the performance regression of the runtime check is tolerable each time, we could just use the template parameter. http://gerrit.cloudera.org:8080/#/c/5115/1/be/src/exec/parquet-common.h File be/src/exec/parquet-common.h: PS1, Line 328: sizeof(T))) re > Hm, I think it's accurate - either it's set (> 0) and fixed, or it's not se It's not that bad but it did take a bit of thought to understand how "fixed length" makes sense when decoding from a variable-length byte array. PS1, Line 338: cimal<Decimal4 > Done. Actually, looking at DecodeFromFixedLenByteArray, it expects this to be >= 1, so we need to check that too. -- To view, visit http://gerrit.cloudera.org:8080/5115 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If95171e65aa48f08b08b8e87f4555dc75e867977 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
