Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21761 )
Change subject: IMPALA-13364: Schema resolution doesn't work for migrated partitioned Iceberg tables that have complex types ...................................................................... Patch Set 7: (3 comments) Thanks for the comments, Mihaly. About decoupling Iceberg related code: I think it's a good idea whenever we can. Currently when we are reading Iceberg tables we mostly re-use the existing classes and methods (basically the HDFS SCAN operator and the file scanners), as ultimately there are regular data files (Parquet, ORC) in an Iceberg tables. We had code for these file formats already, so in most cases it was easier to only add small enhancements to these classes than doing a large and risky refactoring. http://gerrit.cloudera.org:8080/#/c/21761/6/be/src/exec/file-metadata-utils.h File be/src/exec/file-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/21761/6/be/src/exec/file-metadata-utils.h@69 PS6, Line 69: , in any other case > nit: ". In any other case" ? Done http://gerrit.cloudera.org:8080/#/c/21761/6/be/src/exec/file-metadata-utils.h@87 PS6, Line 87: Only valid to call : /// for Iceberg tables. > Can this be reflected in function's name? As it is for other Iceberg specif Done http://gerrit.cloudera.org:8080/#/c/21761/6/be/src/exec/file-metadata-utils.h@89 PS6, Line 89: auto > Is there a consensus on auto usage? It doesn't seem to be used much around We prefer to use the actual type, but here the type is 'flatbuffers::Vector<flatbuffers::Offset<org::apache::impala::fb::FbIcebergPartitionTransformValue>>', which wouldn't be very readable. Also this is a private helper function, not meant to be used outside of this class. -- To view, visit http://gerrit.cloudera.org:8080/21761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie32952021b63d6b55b8820489e434bfc2a91580b Gerrit-Change-Number: 21761 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 30 Sep 2024 16:46:30 +0000 Gerrit-HasComments: Yes
