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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
