impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] REVIEW-ONLY: the results of running clang-format
Date Thu, 18 Aug 2016 17:35:24 GMT
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 <jbapple@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message