Henry Robinson has posted comments on this change. Change subject: REVIEW-ONLY: the results of running clang-format ......................................................................
Patch Set 1: (6 comments) I think this is fine. Some weirdnesses that I noticed in my small sample - I've commented below. The biggest difference to my eye is the early linebreak when function arguments fit on one line (but not the same line as the function name). Seems like an improvement to me, but will take a little getting used to! http://gerrit.cloudera.org:8080/#/c/4046/1/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: PS1, Line 115: "Failed to allocate $0 bytes for scratch row of " : "HashTableCtx.", : scratch_row_size)); multi-line strings seem to be treated like separate arguments, which is fixable - we can just combine the strings once we see what clang-format will try and do. PS1, Line 248: std::min(state->batch_size(), : MAX_EXPR_VALUES_ARRAY_SIZE / expr_values_bytes_per_row_)) this indenting is ugly but an improvement I think in readability: hitting this is a good indicator that we should use a temporary. PS1, Line 896: if (build_expr_ctxs_[i]->root()->type().type != TYPE_STRING && : build_expr_ctxs_[i]->root()->type().type != TYPE_VARCHAR) : continue; This is an issue: we don't want then-clauses on a different line from the if-clause without { }. If it can be fixed automatically, great, if not - let's note it as an exception. Once we fix it manually, clang-format won't revert it. http://gerrit.cloudera.org:8080/#/c/4046/1/be/src/exec/parquet-common.h File be/src/exec/parquet-common.h: PS1, Line 40: // Invalid : parquet::Type::BOOLEAN, // NULL type : parquet::Type::BOOLEAN, parquet::Type::INT32, parquet::Type::INT32, : parquet::Type::INT32, parquet::Type::INT64, parquet::Type::FLOAT, : parquet::Type::DOUBLE, : parquet::Type::INT96, // Timestamp : parquet::Type::BYTE_ARRAY, // String : parquet::Type::BYTE_ARRAY, // Date, NYI : parquet::Type::BYTE_ARRAY, // DateTime, NYI : parquet::Type::BYTE_ARRAY, // Binary NYI : parquet::Type::FIXED_LEN_BYTE_ARRAY, // Decimal : parquet::Type::BYTE_ARRAY, // VARCHAR(N) : parquet::Type::BYTE_ARRAY, // CHAR(N) does this seem weird to anyone else - the mix of packed types and one-per-line statements? (also note that the first two comments aren't aligned in the way that the last lot are). Maybe we shouldn't use trailing comments. http://gerrit.cloudera.org:8080/#/c/4046/1/be/src/util/metrics.h File be/src/util/metrics.h: Line 63: MetricDefs(){}; what's with the missing space here? PS1, Line 221: /// Typically a metric object is cached by its creator after registration. If a metric : /// must : /// be retrieved without an available pointe not sure why it doesn't properly combine lines 222 and 223 -- To view, visit http://gerrit.cloudera.org:8080/4046 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If00bcdb7957ec55a3b8568887de161b7f30d58de Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
