Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12168 )
Change subject: IMPALA-6503: Support reading complex types from ORC ...................................................................... Patch Set 20: (5 comments) Thanks again for the docs! Might be worth to explain it in more detail how you create the row readers, it took me a while to understand it. Maybe extend the comments with a step-by-step walkthrough. So e.g., if I understood it correctly: Resolve columns => get a list of orc::Nodes => get column ids from the nodes => configure orc_row_reader_ with column ids (now they are called types) => create temporary orc::RowReader to get the selected subset of the schema => create impala::OrcColumnReaders http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/hdfs-orc-scanner.h File be/src/exec/hdfs-orc-scanner.h: http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/hdfs-orc-scanner.h@247 PS20, Line 247: 'selected_indices' nit: 'selected_nodes' http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/orc-metadata-utils.h File be/src/exec/orc-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/orc-metadata-utils.h@27 PS20, Line 27: class OrcMetadataUtils { nit: please add some comment to the class and functions. http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/orc-metadata-utils.h@36 PS20, Line 36: class OrcSchemaResolver { nit: please add class comment http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/orc-metadata-utils.h@42 PS20, Line 42: 'pos_field' is true I think it would be clearer to say "'pos_field' is set to true" to emphasize that this is an output parameter. http://gerrit.cloudera.org:8080/#/c/12168/20/be/src/exec/orc-metadata-utils.h@44 PS20, Line 44: 'missing_field' is true "'missing_field' is set to true" -- 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: 20 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: Mon, 04 Mar 2019 17:28:16 +0000 Gerrit-HasComments: Yes
