Dan Hecht has posted comments on this change. Change subject: IMPALA-2418: Display truncated Details column in profile summary ......................................................................
Patch Set 5: (11 comments) As Casey mentioned earlier, you should add some tests. http://gerrit.cloudera.org:8080/#/c/2930/5/be/src/util/summary-util.cc File be/src/util/summary-util.cc: PS5, Line 113: WebUI nit: end with period. http://gerrit.cloudera.org:8080/#/c/2930/5/be/src/util/table-printer.cc File be/src/util/table-printer.cc: PS5, Line 54: CONTINUE_LEN if we're going to have a const for this, we might as well also have one for the string itself: const string CONTINUE = "..."; const int CONTINUE_LEN = CONTINUE.size(); and then use CONTINUE in the places below. PS5, Line 66: COLUMN_PAD nit: here and elsewhere, it's nice to end sentences in comments with periods. Line 67: tmp << string(colpad, ' '); this doesn't look right. colpad will still be 0 at this point when i == 1. how does this work? it be much easier to follow if you just derive colpad from i. Then, just recompute it below based on the number of columns. It's also not immediately clear that the padding is accounted for by the column that follows it, rather than the padding following the column. a comment at line 104 calling that out might help. I also wonder if accounting the padding after the column would be simpler (i.e the last column has no padding). but you don't have to change that if you feel the current way is simplest. PS5, Line 78: last_col_max_width why "max"? isn't this just the width of the last column (without the preceding padding)? PS5, Line 80: last_col_max_width_minus_continue this name doesn't tell us what it really means. how about: // Each continuation (except for the final) will end with "...". int available_width = last_col_width - CONTINUE_LEN; PS5, Line 82: offset_str how about padding_str (and then you can removing comment at line 86. Line 83: for (int loc = last_col_max_width_minus_continue; loc < last_col_size; add a comment above this loop: // Break the last column into 'available_width' chunks. PS5, Line 84: ( remove parenthesis. PS5, Line 88: ( remove parenthesis PS5, Line 91: last_col_max_width this is confusing. if we get into this case, it means that we can fit the rest in 'last_col_max_width_minus_continue' characters. so, we can still use the same substr() arguments for this case. you should pull the substr() part before the if-stmt, and just have the if-stmt control whether "---" is printed. It's extra confusing because we always increment by last_col_max_width_minus_continue on line 84 yet here we may have printed more characters. -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Peter Ebert Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Peter Ebert Gerrit-HasComments: Yes
