Grant Henke has posted comments on this change. ( )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Patch Set 28:

File src/kudu/client/schema.h:
PS28, Line 244:   /// This constructor is private because clients should use 
the Builder API.
              :   ///
              :   /// @param [in] name
              :   ///   The name of the column.
              :   /// @param [in] type
              :   ///   The type of the column.
              :   /// @param [in] is_nullable
              :   ///   Whether the column is nullable.
              :   /// @param [in] default_value
              :   ///   Default value for the column.
              :   /// @param [in] attributes
              :   ///   Column storage attributes.
> nit: usually we don't document private methods since they are not accessibl
File src/kudu/client/
PS28, Line 190:   delete data_;
              :   data_ = new Data(*other.data_);
> What if other == *this?
All instances of CopyFrom in behave this way. The = operator does 
have the check before using it.
File src/kudu/common/schema.h:
PS28, Line 91:  public:
> nit: it's public by default in struct, no need to specify explicitly
I was following "convention" of the other structs in this file. Should we 
define this as style rule?
File src/kudu/integration-tests/
PS28, Line 52: const int kNumServers = 3;
> nit: it's 3 tablet server by default, you could drop this.
Further down I set num_replicas to kNumServers.
PS28, Line 55: {}, {}, kNumServers
> nit: I think it's possible to drop all this and use the parameters 'by defa
Further down I set num_replicas to kNumServers.
PS28, Line 98:   client_->OpenTable(kTableName, &table);
> nit: ASSERT_OK ?
PS28, Line 130:   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
> nit: it's possible to drop this since SetFaultTolerant() set the read mode
PS28, Line 172:     }
> nit: would it make sense to add an 'erroneous case' trying to read decimal

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 28
Gerrit-Owner: Grant Henke <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Grant Henke <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:35:36 +0000
Gerrit-HasComments: Yes

Reply via email to