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 Thu, 17 Nov 2016 00:39:21 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5115/1/be/src/exec/parquet-common.h
File be/src/exec/parquet-common.h:

Line 203:   /// assumed to be BYTE_ARRAY, otherwise it is assumed to be FIXED_LEN_BYTE_ARRAY.
Document return value?


PS1, Line 328: fixed_len_size
This name seems inaccurate since it can be variable. Maybe 'encoded_size'?


PS1, Line 338: fixed_len_size
What if fixed_len_size is negative?


http://gerrit.cloudera.org:8080/#/c/5115/1/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

Line 150:   # Decimal Parquet encodings written by Java Parquet library.
If you're going to create the table as part of the test, might as well copy the data as part
of the test (and avoid the need to reload test data). E.g. like https://github.com/apache/incubator-impala/blob/master/tests/query_test/test_scanners.py#L248


http://gerrit.cloudera.org:8080/#/c/5115/1/testdata/workloads/functional-query/queries/QueryTest/decimal-encodings.test
File testdata/workloads/functional-query/queries/QueryTest/decimal-encodings.test:

PS1, Line 1: ====
           : ---- QUERY
           : select col2 from decimal_encodings;
           : ---- RESULTS
           : 123.00
           : 123.00
           : 123.00
           : 123.00
           : 123.00
           : ---- TYPES
           : DECIMAL
> We're working on getting some richer test data - it's just a bit of a pain 
Sounds good, I think we need a bit more, especially boundary cases for different decimal sizes.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message