impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet
Date Tue, 26 Sep 2017 23:49:37 GMT
Tim Armstrong has posted comments on this change. ( )

Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet

Patch Set 5:

Commit Message:
PS5, Line 7: IMPALA-5307
> Can you add a line at the bottom what the other part(s) would look like?
PS5, Line 56: +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
> Nit: You could make the second column smaller to make this more readable, a
File be/src/exec/
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().
File be/src/exec/parquet-column-readers.h:
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.
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.
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.
File be/src/exec/
PS5, Line 1075:               uncompressed_size, "uncompressed variable-length data", &copy_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.
PS5, Line 1105:     *buffer = data_page_pool_->TryAllocate(size);
Bad indentation.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Tue, 26 Sep 2017 23:49:37 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message