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 20: (5 comments) > 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 Yes, you are right! Sorry for taking you too much time to understand it. I just add such kind of comments before class HdfsOrcScanner. Feel free to correct my comments. 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' Done 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. Done 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 Done 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 emphasiz Good point! Done 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" 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: 20 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Mar 2019 15:54:06 +0000 Gerrit-HasComments: Yes
