Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20548 )
Change subject: IMPALA-12308: DIRECTED distribution mode for V2 Iceberg tables ...................................................................... Patch Set 10: (13 comments) Thank you for the updates Gabor, I have left some further comments. http://gerrit.cloudera.org:8080/#/c/20548/4/be/src/exec/data-sink.h File be/src/exec/data-sink.h: http://gerrit.cloudera.org:8080/#/c/20548/4/be/src/exec/data-sink.h@90 PS4, Line 90: protected: nit: a new line was removed http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/fragment-state.h File be/src/runtime/fragment-state.h: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/fragment-state.h@242 PS10, Line 242: p nit: P http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/fragment-state.cc File be/src/runtime/fragment-state.cc: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/fragment-state.cc@97 PS10, Line 97: nit: empty line http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/fragment-state.cc@131 PS10, Line 131: filename_to_hosts_[full_file_path] Inserting full_file_path to a filename_to_hosts map looks a bit odd. http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/krpc-data-stream-sender.h@193 PS10, Line 193: Funtions nit: Functions http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/krpc-data-stream-sender.h@197 PS10, Line 197: i nit: I http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/query-state.h@135 PS10, Line 135: int int32 to make it a bit more aligned with admission_control_service.proto description. http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/runtime/query-state.h@172 PS10, Line 172: const http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/scheduling/scheduler.h@86 PS10, Line 86: /// Map from filenames to hosts where those files are scheduled, grouped by scan node : // ID. nit: one line or the second line needs /// http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/20548/10/be/src/scheduling/scheduler.cc@980 PS10, Line 980: ByNodeFilenamesToHosts duplicate_check; Unused variable, maybe even the parameter in PopulateFilenamesToHostsMapping. http://gerrit.cloudera.org:8080/#/c/20548/10/common/thrift/Partitions.thrift File common/thrift/Partitions.thrift: http://gerrit.cloudera.org:8080/#/c/20548/10/common/thrift/Partitions.thrift@44 PS10, Line 44: of nit: for http://gerrit.cloudera.org:8080/#/c/20548/10/fe/src/main/java/org/apache/impala/planner/DataPartition.java File fe/src/main/java/org/apache/impala/planner/DataPartition.java: http://gerrit.cloudera.org:8080/#/c/20548/10/fe/src/main/java/org/apache/impala/planner/DataPartition.java@79 PS10, Line 79: public final static DataPartition DIRECTED = new DataPartition(TPartitionType.DIRECTED); nit: I think this would look more consistent with the class structure in L70 http://gerrit.cloudera.org:8080/#/c/20548/10/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/20548/10/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@79 PS10, Line 79: } nit: missing empty line -- To view, visit http://gerrit.cloudera.org:8080/20548 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I212afd7c9e94551a1c50a40ccb0e3c1f7ecdf3d2 Gerrit-Change-Number: 20548 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 09 Nov 2023 16:22:41 +0000 Gerrit-HasComments: Yes
