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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
