Michael Smith 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 9: Code-Review+1 (15 comments) This makes sense to me. http://gerrit.cloudera.org:8080/#/c/18219/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18219/8//COMMIT_MSG@67 PS8, Line 67: a block with spill id 1 and sequence number 1. Instead, if step > Added examples. Simply, the idea is to specify which future block is going Done http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-file.h File be/src/runtime/io/disk-file.h: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-file.h@256 PS8, Line 256: // written_bytes() in FileWriter is thread-safe. > The life cycle of file_writer_ should be the same as the DiskFile, nobody e Done http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-io-mgr.cc@291 PS8, Line 291: DCHECK_LE(written_bytes, actual_file_size); > Done Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-io-mgr.cc@413 PS8, Line 413: Status RemoteOperRange::DoFetch() { > Thanks for pointing this out. I think it was used within the function, but Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/disk-io-mgr.cc@435 PS8, Line 435: // Return okay because the remote file is deleted only when the query is going to > That makes sense. Changed. Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/io/request-context.h@195 PS8, Line 195: void set_read_use_batch_counter(RuntimeProfile::Counter* read_use_batch_counter) { > Done Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr-internal.h File be/src/runtime/tmp-file-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr-internal.h@67 PS8, Line 67: is_full_ = true; > Yeah, that is reason to make it easier to manage the blocks and have a fixe Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr-test.cc File be/src/runtime/tmp-file-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr-test.cc@334 PS8, Line 334: TmpFileGroup& file_group, int idx, bool batch_read = false) { > Added one old testcase to use it, also changed the new testcase to use both Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr-test.cc@2148 PS8, Line 2148: WaitForCallbacks(1); > That is a good idea. Added. Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.h@521 PS8, Line 521: /// compression). The caller should not modify the data in 'buffer' until the write > Changed to default value. Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.cc@608 PS8, Line 608: > There is a context that we can reserve the local disk space for a new file Done http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.cc@831 PS8, Line 831: *path = parsed_raw_path; > Done. Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/runtime/tmp-file-mgr.cc@1362 PS8, Line 1362: } > Done Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/service/query-options.h@279 PS8, Line 279: QUERY_OPT_FN(remote_batch_read_prefetch_step, REMOTE_BATCH_READ_PREFETCH_STEP, \ > That is a good point. Filed a Jira IMPALA-11656 to follow up. Ack http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/18219/8/be/src/service/query-options.cc@1255 PS8, Line 1255: "Only integer value larger than or equal to 0 is allowed.", > Done Ack -- 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: 9 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 20 Oct 2022 17:58:00 +0000 Gerrit-HasComments: Yes
