impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2494: Support for byte array-encoded decimals in Parquet scanner
Date Mon, 21 Nov 2016 20:06:46 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2494: Support for byte array-encoded decimals in Parquet scanner

Patch Set 3:

File be/src/exec/

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If95171e65aa48f08b08b8e87f4555dc75e867977
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message