Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711012053.vA1Kr8KH023679@ip-10-146-233-104.ec2.internal>
Date: Wed, 1 Nov 2017 20:53:08 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Bikramjeet Vig <bikramjeet.vig@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>, Lars Volker <lv@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2494=3A_Support_for_byte_array_encoded_decimals_in_Parquet_scanner=0A?=
In-Reply-To: <gerrit.1503621477000.I2c0e881045109f337fecba53fec21f9cfb9e619e@gerrit.cloudera.org>
References: <gerrit.1503621477000.I2c0e881045109f337fecba53fec21f9cfb9e619e@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com, lv@cloudera.com,
        marcelk@gmail.com, dhecht@cloudera.com,
        reviews@impala.incubator.apache.org, bikramjeet.vig@cloudera.com,
        mjacobs@apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="i94z0X8Xx/o="; charset=UTF-8
archived-at: Wed, 01 Nov 2017 20:53:16 -0000

--i94z0X8Xx/o=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Tim Armstrong has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/7822 )

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


Patch Set 7:

(2 comments)

I think we'll need to d=
o some more work on testing for the int32/64 patches - we don't have pre-ex=
isting test files from parquet-mr=2E I think we'll have to generate some mo=
re test files with parquet-mr for the other cases, and we could consider tu=
rning that code into a data generator to generate more test files=2E From w=
hat I could tell Hive doesn't have a knob to generate some of these alterna=
tive output encodings=2E

 I feel ok with the coverage since we have end-to=
-end tests then more exhaustive unit tests for the various ways of encoding=
 it=2E

http://gerrit=2Ecloudera=2Eorg:8080/#/c/7822/7/be/src/exec/parquet-=
common=2Eh
File be/src/exec/parquet-common=2Eh:

http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/7822/7/be/src/exec/parquet-common=2Eh@391
PS7, Line 391: fi=
xed_len_size
Looked again=2E The variable name (and recycling the argument =
storage) is confusing=2E Maybe 'encoded_byte_size'?


http://gerrit=2Ecloud=
era=2Eorg:8080/#/c/7822/7/be/src/exec/parquet-plain-test=2Ecc
File be/src/e=
xec/parquet-plain-test=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/7822/=
7/be/src/exec/parquet-plain-test=2Ecc@33
PS7, Line 33: int EncodeVarLenDeci=
mal(const DECIMAL_TYPE& t, int fixed_len_size, uint8_t* buffer){
I took ano=
ther look at the standard and it says that the minimum number of bytes requ=
ired to store the unscaled value should be used: https://github=2Ecom/apach=
e/parquet-format/blob/master/LogicalTypes=2Emd#decimal

I think this means =
that we should not be including any preceding "0" bytes=2E I=2Ee=2E we shou=
ld not have a fixed_len_size argument and instead determine the size based =
on the number of leading zero bytes in the value=2E


-- 
To view, visit h=
ttp://gerrit=2Ecloudera=2Eorg:8080/7822
To unsubscribe, visit http://gerrit=
=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: =
master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c0e881045109f337fecb=
a53fec21f9cfb9e619e
Gerrit-Change-Number: 7822
Gerrit-PatchSet: 7
Gerrit-Ow=
ner: Bikramjeet Vig <bikramjeet=2Evig@cloudera=2Ecom>
Gerrit-Reviewer: Bikr=
amjeet Vig <bikramjeet=2Evig@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dh=
echt@cloudera=2Ecom>
Gerrit-Reviewer: Lars Volker <lv@cloudera=2Ecom>
Gerri=
t-Reviewer: Matthew Jacobs <mjacobs@apache=2Eorg>
Gerrit-Reviewer: Tim Arms=
trong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Wed, 01 Nov 2017 20:=
53:08 +0000
Gerrit-HasComments: Yes

--i94z0X8Xx/o=--