Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8085 )
Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-5307 > Can you add a line at the bottom what the other part(s) would look like? Done http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG@56 PS5, Line 56: +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+ > Nit: You could make the second column smaller to make this more readable, a Done http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc@245 PS5, Line 245: context_->ReleaseCompletedResources(nullptr, true); > I think it's best to change the whole file at once, or only change occurren I fixed it while I was checking the callsites of ReleaseCompletedResources(). http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h File be/src/exec/parquet-column-readers.h: http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@476 PS5, Line 476: Status AllocateUncompressedDataPage( > Should we call this "AllocateUncompressedDataBuffer"? Otherwise it sounds t That seems vaguer. It would also be confusing since the pool is data_page_pool_. I updated the comment to mention that the contents are uncompressed rather than the page. http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@477 PS5, Line 477: int64_t size, const std::string& desc, uint8_t** buffer); > Maybe err_desc, err_detail, or detail? "desc" reminds me of descriptors. Done. Also switch to const char* so we don't need to create a std::string for every call. http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@485 PS5, Line 485: IsStringType > This does not say "VarLenStringType" but above in a comment you refer to va Fixed it. We don't need to copy if it's a CHAR. http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.cc File be/src/exec/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.cc@1075 PS5, Line 1075: uncompressed_size, "uncompressed variable-length data", ©_buffer)); > DCHECK(copy_buffer != nullptr); And maybe initialize it to nullptr, so that It doesn't seem that necessary to me since it's part of the contract of the function and it will crash immediately anyway. http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.cc@1105 PS5, Line 1105: *buffer = data_page_pool_->TryAllocate(size); Bad indentation. -- To view, visit http://gerrit.cloudera.org:8080/8085 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2 Gerrit-Change-Number: 8085 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 26 Sep 2017 23:49:37 +0000 Gerrit-HasComments: Yes
