Taras Bobrovytsky has posted comments on this change. Change subject: MPALA-5776: Write partial tuple to the correct mempool ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/7639/2//COMMIT_MSG Commit Message: Line 7: MPALA-5776: Write partial tuple to the correct mempool > Missing I. Done http://gerrit.cloudera.org:8080/#/c/7639/2/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 169: row_batch->tuple_data_pool()->AcquireData(boundary_pool_.get(), false); > Does this even make sense? are there any cases where it's valid for the row I don't think that data from the boundary pool is ever used outside of this class, so it makes sense to clear it. I analyzed the code, and it doesn't look to me like data in boundary pool is ever used, but how can we be 100% sure? Line 269: bool eosr = true; > Remove. This not needed/used. It's used on line 277. Are you suggesting to remove it from the signature of FillByteBuffer as well? Line 299: boundary_column_.Clear(); > What's the point of clearing the boundary column here? I don't think the sc Removed this. PS2, Line 765: batches > I'm not sure that I understand the reference to batches. Do they mean block Yeah, "batches" doesn't make sense here. Changed it to blocks. Line 806: partial_tuple_->DeepCopy(tuple_, *scan_node_->tuple_desc(), pool); > Can you add a column clarifying what the intent is. E.g. "Copy over all mat Done Line 808: boundary_row_.Reset(); > Another idea: Add a separate function for copying the partial tuple into th I added a new mem pool, so boundary row and columns are not affected any more. Alex, do you guys think it's still worth creating a separate function? PS2, Line 810: emptied > cleared? The comment makes me thing that all the memory has been freed, but Done Line 814: partial_tuple_ = Tuple::Create(tuple_byte_size_, boundary_pool_.get()); > Is it possible to just initialize partial_tuple_ when it's needed? I.e. bef Done Line 896: int num_fields, bool copy_strings) { > Can you remove the 'copy_strings' parameter? It's not used in this function Done http://gerrit.cloudera.org:8080/#/c/7639/2/be/src/exec/hdfs-text-scanner.h File be/src/exec/hdfs-text-scanner.h: Line 188: /// the boundary pool. > Hah! If only the code had matched the comment... totally agree Line 194: /// Mem pool for boundary_row_ and boundary_column_. > Needs updating since it also backs partial_tuple_. I made changes in the next patch so that this comment is now true. -- To view, visit http://gerrit.cloudera.org:8080/7639 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
