Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20010 )
Change subject: IMPALA-11996: Scanner change for Iceberg metadata querying ...................................................................... Patch Set 9: (18 comments) Looked at the C++ parts. Tomorrow I'll continue with the Java parts. http://gerrit.cloudera.org:8080/#/c/20010/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20010/2//COMMIT_MSG@7 PS2, Line 7: ta queryi typo: metadata http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/hbase/hbase-table-scanner.cc File be/src/exec/hbase/hbase-table-scanner.cc: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/hbase/hbase-table-scanner.cc@a128 PS9, Line 128: Unintended delete? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@32 PS9, Line 32: scans redundant word 'scans' http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@34 PS9, Line 34: scans tables? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@35 PS9, Line 35: predefined nature of the metadata : /// tables Maybe 'virtual nature' of the metadata tables? I mean they don't exist physically on the filesystem, they are only provided via the Iceberg APIs. http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@40 PS9, Line 40: rowsn typo: rows http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@45 PS9, Line 45: objec typo: object http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@50 PS9, Line 50: StrutcLike StructLike http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@51 PS9, Line 51: converts materializes converts and materializes? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@54 PS9, Line 54: /// This scan node should be executed on the coordinator, because it depends on the : /// frontend's table cache. Will we be able to execute complex plans with multiple JOINs and aggregations with this approach? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@58 PS9, Line 58: virtual virtual keyword is redundant here and below as well. http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@67 PS9, Line 67: Create Creates http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@85 PS9, Line 85: inline Do we need the inline keyword here and below? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@99 PS9, Line 99: std::unique_ptr Do we need the unique_ptr here? I.e. can't we just have a simple member? http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@107 PS9, Line 107: /// Tuple index in tuple row. : int tuple_idx_ = 0; In SCANs the tuple rows always only have a single tuple AFAIK. Or make it a constexpr. http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc@71 PS9, Line 71: NULL We prefer nullptr http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.h File be/src/exec/iceberg-metadata/iceberg-row-reader.h: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.h@25 PS9, Line 25: using namespace std; We shouldn't use 'using' at global or namespace-scope in headers. http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.h@50 PS9, Line 50: inline Do we need the inline keyword? -- To view, visit http://gerrit.cloudera.org:8080/20010 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e943cecd77f5ef7af7cd07e2b596f2c5b4331e7 Gerrit-Change-Number: 20010 Gerrit-PatchSet: 9 Gerrit-Owner: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Anonymous Coward <lipeng...@apache.org> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <g.furnst...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 20:08:41 +0000 Gerrit-HasComments: Yes
