Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18219 )
Change subject: IMPALA-11064 Optimizing Temporary File Structure for Batch Reading ...................................................................... Patch Set 5: (8 comments) Thanks Michael for the review. http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/exec/partitioned-hash-join-builder.h@547 PS4, Line 547: return null_aware_partition_.get(); > This appears to be duplicated in partitioned-hash-join-node.h. There's noth Done http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/bufferpool/buffer-pool.h@412 PS4, Line 412: DISALLOW_COPY_AND_ASSIGN(ClientHandle); > I don't see why this is needed. Removed. http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/bufferpool/buffer-pool.h@538 PS4, Line 538: uint64_t spill_id() const { return spill_id_; } > Different styles for setters and getters seems odd, but I'll assume this is Done http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/disk-file.h File be/src/runtime/io/disk-file.h: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/disk-file.h@20 PS4, Line 20: #include <string> > Why is this inclusion needed? Removed. http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/disk-file.h@569 PS4, Line 569: > Why switch all these from atomic to locking? I introduced these atomic variables several patches ago, but on second thought, maybe it is more clear here to access the numbers under a certain lock instead of using a lot of atomic numbers, and more flexible, can do more operations under the lock. Since it involves the IO, the performance should not make much difference. http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/local-file-writer.cc File be/src/runtime/io/local-file-writer.cc: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/local-file-writer.cc@92 PS4, Line 92: if (written_bytes != nullptr) *written_bytes = written_bytes_; > We only update written_bytes_ if you pass in a ref to get the value? Could Done http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/io/request-context.cc@577 PS4, Line 577: // It is possible that the RequestContext is in Inactive state for remote ranges. > This needs some explanation. Why is it different from AddWriteRange and IsC Added some comments. Previously we don't allow any access to the context in inactive state, it has an assert later in the code. The reason for the change is the new added TmpFileUploader may try to add the upload range even after the request context inactive. Because during TmpFileGroup::Close(), it would inactive the context first before deleting the files for some reason, while the TmpFileUploader would keep a lock of the file protecting it from deleting before adding the range to the context, however, it would be safe for the TmpFileUploader to receive an error from the context and give up the upload because the TmpFileGroup is going to close and no need for the upload anymore. http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18219/4/be/src/runtime/tmp-file-mgr.cc@123 PS4, Line 123: "would be the nth power of 2, the unit is MB."); > End sentence with a period for consistency. Done -- To view, visit http://gerrit.cloudera.org:8080/18219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If913785cac9e2dafa20013b6600c87fcaf3e2018 Gerrit-Change-Number: 18219 Gerrit-PatchSet: 5 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 16 Jun 2022 06:30:07 +0000 Gerrit-HasComments: Yes
