Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18240 )
Change subject: IMPALA-11053: Impala should be able to read migrated partitioned Iceberg tables ...................................................................... Patch Set 3: (6 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/18240/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18240/2//COMMIT_MSG@13 PS2, Line 13: ab > nit: 'be' is not needed Done http://gerrit.cloudera.org:8080/#/c/18240/2//COMMIT_MSG@37 PS2, Line 37: s a > nit: "s are" Done http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/file-metadata-utils.cc File be/src/exec/file-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/file-metadata-utils.cc@19 PS2, Line 19: > nit: empty line We usually have an empty line after the header file corresponding to the current .cc file. http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/file-metadata-utils.cc@69 PS2, Line 69: const ColumnDescriptor& col_desc = : scan_node_->hdfs_table() > Should be moved after L58. Done http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/orc-column-readers.cc@470 PS2, Line 470: Parse error in possibly corrupt ORC file: '$0' > nit: this is not part of this change, but is it possible to give a more spe Added an extra sentence to the error message. http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/parquet/hdfs-parquet-scanner.cc File be/src/exec/parquet/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18240/2/be/src/exec/parquet/hdfs-parquet-scanner.cc@119 PS2, Line 119: HdfsColumnarScanner > Just a question, what is the impact/need of using this class instead of its It's good practice to always use the direct parent, even if it doesn't define the method. Because once it defines it we would want to invoke it automatically. I just run into this issue in an earlier version of this CR: I defined HdfsColumnarScanner::Open() and it didn't got invoked, only HdfsScanner::Open(). I also updated HdfsOrcScanner::Open(). -- To view, visit http://gerrit.cloudera.org:8080/18240 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac11a02de709d43532056f71359c49d20c1be2b8 Gerrit-Change-Number: 18240 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 24 Feb 2022 18:52:58 +0000 Gerrit-HasComments: Yes
