Henry Robinson has posted comments on this change. Change subject: IMPALA-2418 Display truncated Details column in profile summary ......................................................................
Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/2930/3//COMMIT_MSG Commit Message: Line 7: IMPALA-2418 Display truncated Details column in profile summary nit: colon after jira: IMPALA-2418: http://gerrit.cloudera.org:8080/#/c/2930/3/be/src/util/summary-util.cc File be/src/util/summary-util.cc: Line 113: printer.set_max_output_width(38); Comment on how you chose this parameter. 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 PrintRow. Just have a const int in that method. Line 56: int colpad = 0; move this declaration to line 68: int colpad = (i == 1) ? COLUMN_PAD : 0; Line 66: //set padding between columns, 0 for first column, then COLUMN_PAD Format comments like this: // Set padding between... Line 76: //if last column overflows, write remainder underneath on next line Comment formatting. Line 77: row.size() - 1 prefer: int last_col_size = row.back().size(); Line 78: widths[row.size() - 1] widths.back() - 1 Line 79: if(last_col_size > last_col_max_width) { space after if Line 82: last_col_max_width - CONTINUE_LEN declare a variable with this value, since it's used four times in the loop Line 86: for (int pad = 0; pad < offset; ++pad) { more readable to write: ss << string(offset, ' '); and probably best of all to have: string offset_str(offset, ' '); declared on line 81 since it doesn't change between loops. Line 89: if(loc + (last_col_max_width - CONTINUE_LEN) < row[row.size() - 1].length()) { space after if Line 89: row[row.size() - 1].length() this is last_col_size, isn't it? 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. -- 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-HasComments: Yes
