Will Berkeley has posted comments on this change. Change subject: [WIP] KUDU-721 Support for Decimal type: Part 1 (C++) ......................................................................
Patch Set 2: (16 comments) I, for one, welcome our new bot overlord. http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: Line 264: Status TestFunc(const MonoTime& deadline, bool* retry, int* counter) { > warning: parameter 'deadline' is unused [misc-unused-parameters] Pass http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/client/scan_batch.cc File src/kudu/client/scan_batch.cc: Line 184: } else if (col.type_info()->type() == DECIMAL64) { > warning: don't use else after return [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/client/scan_batch.h File src/kudu/client/scan_batch.h: Line 164: Status GetUnixTimeMicros(const Slice& col_name, int64_t* micros_since_utc_epoch) > warning: function 'kudu::client::KuduScanBatch::RowPtr::GetUnixTimeMicros' Done Line 196: Status GetUnixTimeMicros(int col_idx, int64_t* micros_since_utc_epoch) > warning: function 'kudu::client::KuduScanBatch::RowPtr::GetUnixTimeMicros' Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/decimal_value-test.cc File src/kudu/common/decimal_value-test.cc: Line 187: DecimalValue::MAX_UNSCALED_DECIMAL32 + 1, &overflow); > warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: Line 283: return Set<TypeTraits<DECIMAL32> >(col_idx, (int32_t) val.value()); > warning: C-style casts are discouraged; use static_cast [google-readability Done Line 284: } else if (col.type_info()->type() == DECIMAL64) { > warning: don't use else after return [readability-else-after-return] Done Line 612: } else if (col.type_info()->type() == DECIMAL64) { > warning: don't use else after return [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: Line 93: Status SetUnixTimeMicros(const Slice& col_name, > warning: function 'kudu::KuduPartialRow::SetUnixTimeMicros' has a definitio Done Line 123: Status SetUnixTimeMicros(int col_idx, int64_t micros_since_utc_epoch) WARN_UNUSED_RESULT; > warning: function 'kudu::KuduPartialRow::SetUnixTimeMicros' has a definitio Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/schema.cc File src/kudu/common/schema.cc: Line 20: #include <set> > warning: #includes are not sorted properly [llvm-include-order] Done Line 73: } else { > warning: don't use else after return [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/schema.h File src/kudu/common/schema.h: Line 170: attributes_(std::move(attributes)), > warning: std::move of the variable of a trivially-copyable type has no effe Done Line 171: storage_attributes_(std::move(storage_attributes)) { > warning: std::move of the variable of a trivially-copyable type has no effe Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: Line 247: const ColumnAttributesPB attributesPB = pb.column_attributes(); > warning: the const qualified variable 'attributesPB' is copy-constructed fr Done http://gerrit.cloudera.org:8080/#/c/5104/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 866: ColumnSchema col = client_schema.column(i); > warning: the variable 'col' is copy-constructed from a const reference but Done -- To view, visit http://gerrit.cloudera.org:8080/5104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f7829cdad883f5c124a437327062b2b45b6841b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
