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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Peter Ebert Gerrit-HasComments: Yes
