impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3671: Add query option to limit scratch space usage
Date Wed, 17 Aug 2016 18:04:24 GMT
Bikramjeet Vig has posted comments on this change.

Change subject: IMPALA-3671: Add query option to limit scratch space usage

Patch Set 1:

File be/src/runtime/

Line 216:     disable_spill_(state->query_ctx().disable_spilling || block_write_threshold_
== 0),
> There's a subtle bug here if you have scratch_limit = 0, because the block 

Line 245:       block_mgr->reset(
> Don't need newline after reset(

Line 827:       return status;
> Add a brief comment. Something like:
File be/src/runtime/buffered-block-mgr.h:

PS1, Line 584: tmp_file_grp
> maybe just tmp_file_group? It will be hard to remember the abbreviation.
File be/src/runtime/

Line 224:       && fileGroup_->current_write_size_ + write_size
> && should go on previous line, and I think this condition fits on one line.

Line 226:     return Status(TErrorCode::SCRATCH_LIMIT_EXCEEDED);
> Let's include the limit value in the error message, will make it easier for

PS1, Line 271: AddFile
> AddNewFile()?

Line 287:     file.Remove();
> I think we need to call delete on these files: in the tmp-file-mgr.h it say
there is not need to delete explicitly as tmp_files.clear() destroys all objects in the container

PS1, Line 292: Size
> Maybe NumFiles()? Size is a little ambiguous: could mean the total bytes.

Line 296: int64_t TmpFileMgr::FileGroup::SetSpaceAllocationLimit(int64_t max_size){
> missing space before {
File be/src/runtime/tmp-file-mgr.h:

Line 88:     /// The FileGroup this belongs to. Can be null if it does not belong to any group.
> I feel like we should make file_group_ a required field to simplify things,

PS1, Line 89: fileGroup_
> file_group_ (not camel case).

Line 110:   /// Represents a group of files. The total allocated bytes of the group can be
> The wrapping of some of these comments is a little weird - we normally wrap

Line 124:     File* GetFileAt(int index);
> I think these methods need one-line comments.

PS1, Line 126: Remove
> Remove()

Line 129:     int64_t SetSpaceAllocationLimit(int64_t max_size);
> If we're not using the return value anywhere, I'd say just make this void. 

PS1, Line 131: bool
> why bool?

Line 134: 
> re,move extra space.

PS1, Line 141: current_write_size_
> I think the comment would actually be a better variable name: current_space

Line 144:     int64_t max_write_size_;
> max_space_allocated_?
"max space allocated" might be a bit misleading. How does  "max_space_limit_" sound?
File common/thrift/ImpalaService.thrift:

Line 240:   // otherwise specified either as:
> More concise: 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3e9039ad1c0df978ab71b956af33fd997714e73
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <>
Gerrit-Reviewer: Bikramjeet Vig <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message