impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4725: Query option to control Parquet array resolution.
Date Tue, 07 Mar 2017 05:59:41 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4725: Query option to control Parquet array resolution.
......................................................................


Patch Set 1:

(7 comments)

All good suggestions, thanks! Reworked the code accordingly.

http://gerrit.cloudera.org:8080/#/c/6250/1//COMMIT_MSG
Commit Message:

PS1, Line 11: resoution
> resolution
Done


PS1, Line 29: resoution
> resolution
Done


http://gerrit.cloudera.org:8080/#/c/6250/1/be/src/exec/parquet-metadata-utils.cc
File be/src/exec/parquet-metadata-utils.cc:

PS1, Line 409: 3)
> what's this? the number of enum elements? consider defining NUM_ARRAY_ENCOD
Removed this. Doesn't seem necessary with the new static table.


Line 416:   Status one_level_status = Status::OK();
> could be:
Reworked as suggested.


http://gerrit.cloudera.org:8080/#/c/6250/1/be/src/exec/parquet-metadata-utils.h
File be/src/exec/parquet-metadata-utils.h:

Line 144:     GetOrderedArrayEncodings(array_resolution_, &ordered_array_encodings_);
> why not do this in the constructor and don't bother saving array_resolution
Good point. Using a static array now.


Line 190:   void GetOrderedArrayEncodings(TParquetArrayResolution::type array_resolution,
> could be static, then there's no question that array_resolution is the only
I removed this function.


http://gerrit.cloudera.org:8080/#/c/6250/1/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

Line 264:   // it non-default in a compatibility breaking release.
> why want a jira for that, or else we'll likely forget to do it.
Filed IMPALA-5037.


-- 
To view, visit http://gerrit.cloudera.org:8080/6250
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f32e19ec542d4d485154c9d65d0f5e3f9f0a907
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message