Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/12168 )
Change subject: IMPALA-6503: Support reading complex types from ORC ...................................................................... Patch Set 18: (7 comments) http://gerrit.cloudera.org:8080/#/c/12168/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12168/17//COMMIT_MSG@30 PS17, Line 30: Tests: > Did you run exhaustive tests? I think there are some nested types tests tha Passed exhaustive tests in DEBUG and RELEASE build: https://jenkins.impala.io/job/pre-review-test/318/ https://jenkins.impala.io/job/pre-review-test/319/ http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/hdfs-orc-scanner.cc@379 PS17, Line 379: if > Doubt inline provides any measurable benefit it. I'd remove it, but ok to i Done http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/hdfs-orc-scanner.cc@402 PS17, Line 402: pos_slots.pop(); > I think the data flow would be clearer if pos_slots was a local and Resolve Good point! http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-column-readers.cc@30 PS17, Line 30: string > inline seems unnecessary Done http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-metadata-utils.h File be/src/exec/orc-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-metadata-utils.h@48 PS17, Line 48: const HdfsTableDescriptor& tbl_desc_; > All the members are not assigned except for constructor, so you could add a Done http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-metadata-utils.cc File be/src/exec/orc-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-metadata-utils.cc@130 PS17, Line 130: || type.type == TYPE_INT || type.type == TYPE_BIGINT) { > Can you put braces on the multi-line if statements here and below. Done http://gerrit.cloudera.org:8080/#/c/12168/17/be/src/exec/orc-metadata-utils.cc@175 PS17, Line 175: > "Decimal value" instead of "It" Done -- To view, visit http://gerrit.cloudera.org:8080/12168 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I244dc9d2b3e425393f90e45632cb8cdbea6cf790 Gerrit-Change-Number: 12168 Gerrit-PatchSet: 18 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 01 Mar 2019 05:24:00 +0000 Gerrit-HasComments: Yes
