impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1346/1590: fix sorter buffer mgmt when spilling
Date Wed, 01 Jun 2016 20:53:22 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-1346/1590: fix sorter buffer mgmt when spilling
......................................................................


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/2908/5//COMMIT_MSG
Commit Message:

Line 7: IMPALA-1346/1590: fix sorter buffer mgmt when spilling
IMPALA-2344 too?


http://gerrit.cloudera.org:8080/#/c/2908/5/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

Line 1447
why does this go away?


Line 1481: blocks_per_run
how about renaming this pinned_blocks_per_run?


Line 1484: blocks
one or two blocks ..
(when I first read the comment i wondered why we claimed that runs blocks were all pinned)


Line 1497: and may
in case we ..

to clarify that we might not use this output run but don't know yet.


Line 1519:     // Should have errored out before this point if we had only two runs.
not sure what this comment is saying. is it referring to this dcheck in some way?


Line 1530:   DCHECK_GT(max_num_runs, 1);
DCHECK_GE(sorted_runs_.size(), 2) or is that not the case? i guess from MergeIntermediateRuns
we can sometimes end up with the final run "merging" a single sorted run?


Line 1547: set up a merge
have at least two runs ready to merge


Line 1551:       if (merging_runs_.size() >= 2) break;
okay to ignore but I think it's easier to read as:

if (merging_runs_.size() < 2) {
   return status;
}
// Merge the runs we were able to prepare.
break;


http://gerrit.cloudera.org:8080/#/c/2908/5/be/src/runtime/sorter.h
File be/src/runtime/sorter.h:

Line 130: least two runs
is that true? what if on input sorted_runs_.size() == 1 (see my question in .cc file)


-- 
To view, visit http://gerrit.cloudera.org:8080/2908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfe55cc13c7f2b54cba1d05ade44cbcf6bb573c0
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message