Gabor Kaszab 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: (10 comments) Thanks for taking another look! http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/fragment-state.cc File be/src/runtime/fragment-state.cc: http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/fragment-state.cc@90 PS8, Line 90: if (fragment_.output_sink.type == TDataSinkType::DATA_STREAM_SINK) { > nit: might be worth to create a separate method for the body of this if stm Done http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/fragment-state.cc@105 PS8, Line 105: > nit: you could use DCHECK_EQ Done http://gerrit.cloudera.org:8080/#/c/20548/4/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/20548/4/be/src/runtime/krpc-data-stream-sender.h@113 PS4, Line 113: M > I am OK to leave it as is, but this thread seems unresponded. I figured I'd just keep it as it is, but since it was brought up this many times and this is just a comma, I don't mind adding it. http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/krpc-data-stream-sender.cc@1084 PS8, Line 1084: DCHECK > nit: could be DCHECK_EQ Done http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/krpc-data-stream-sender.cc@1102 PS8, Line 1102: // This can happen when e.g. compaction removed some data files from a snapshot : // but a delete file referencing them remained because it references other data : // files that remains in the new snapshot. > We could also mention table sampling. Done http://gerrit.cloudera.org:8080/#/c/20548/8/be/src/runtime/krpc-data-stream-sender.cc@1119 PS8, Line 1119: DumpDestinationHosts(); : : stringstream ss; : ss << "Failed to distribute Iceberg delete file content" > nit: We usually use strings::Substitute() instead of stringstream. The reason I had to do this way is that NetworkAddressPB doesn't have a toString() or similar method that I could use to pass the stringified version to the Substitute() function. It however gives you a '<<' operator to use for string conversion, so stringstream seemed the easiest solution here. http://gerrit.cloudera.org:8080/#/c/20548/8/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/20548/8/common/protobuf/control_service.proto@400 PS8, Line 400: it > typo: it Done http://gerrit.cloudera.org:8080/#/c/20548/8/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/20548/8/fe/src/main/java/org/apache/impala/planner/JoinNode.java@218 PS8, Line 218: | > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/20548/9/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-position-deletes.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-position-deletes.test: http://gerrit.cloudera.org:8080/#/c/20548/9/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-read-position-deletes.test@745 PS9, Line 745: Since there was a fix to take into account all the test configurations, now these tests are running both with the V2 read optimisation ON and OFF. I have to move these tests out from this file because they are only relevant for the optimisations being ON. http://gerrit.cloudera.org:8080/#/c/20548/8/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/20548/8/tests/query_test/test_iceberg.py@1199 PS8, Line 1199: > Why do we need this annotation? because in the new tests I change MT_DOP and the size of a scan range than might have a sideeffect on other tests that are running in parallel with this. Anyway, I moved the new tests into a another class (see my other comment in the .test file) so I removed this annotation -- 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 <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 09 Nov 2023 13:09:31 +0000 Gerrit-HasComments: Yes
