impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] REVIEW-ONLY: the results of running clang-format
Date Thu, 18 Aug 2016 18:17:32 GMT
Jim Apple has posted comments on this change.

Change subject: REVIEW-ONLY: the results of running clang-format
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4046/1/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

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 i
I don't see a way to get clang-format to fix that.


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-l
Yeah, this is not great. I don't see a way to fix this in clang-format's config, unfortunately.


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?
Not sure. I don't see any config parameters to tune this.


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
I've noticed this as well. I think the reasoning is that the user put a line break in the
comment, maybe they really meant it.


-- 
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