impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peter Ebert (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2418: Display truncated Details column in profile summary
Date Wed, 01 Jun 2016 21:40:41 GMT
Peter Ebert has posted comments on this change.

Change subject: IMPALA-2418: Display truncated Details column in profile summary
......................................................................


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/2930/3/be/src/util/table-printer.cc
File be/src/util/table-printer.cc:

Line 28: const int CONTINUE_LEN = 3;
> no need to have this at file-scope because it's not used outside of PrintRo
Done


Line 56:   int colpad = 0;
> move this declaration to line 68:
is this not more expensive since we set the value each time the for loop executes? just curious

also, colpad is needed outside of the for loop, if the table has only one column you need
to track if there is padding or not (line 78).


Line 66:     //set padding between columns, 0 for first column, then COLUMN_PAD
> Format comments like this:
Done


Line 76:   //if last column overflows, write remainder underneath on next line
> Comment formatting.
Done


Line 77: row.size() - 1
> prefer:
Done


Line 78: widths[row.size() - 1]
> widths.back() - 1
Done


Line 79:   if(last_col_size > last_col_max_width) {
> space after if
Done


Line 86:       for (int pad = 0; pad < offset; ++pad) {
> more readable to write:
Done


Line 89: row[row.size() - 1].length()
> this is last_col_size, isn't it?
Done


Line 89:       if(loc + (last_col_max_width - CONTINUE_LEN) < row[row.size() - 1].length())
{
> space after if
Done


http://gerrit.cloudera.org:8080/#/c/2930/3/be/src/util/table-printer.h
File be/src/util/table-printer.h:

Line 56:   /// Helper function to print one row to ss.
> Comment what 'total_width' means.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I228057dc134cb46673d8370ff859c00cd9739cd4
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Peter Ebert
Gerrit-Reviewer: Casey Ching <casey@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Peter Ebert
Gerrit-HasComments: Yes

Mime
View raw message