Alexey Serbin has posted comments on this change. ( )

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

Patch Set 28:


LGTM, just a few nits.
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 accessible 
via client API.
File src/kudu/client/
PS28, Line 190:   delete data_;
              :   data_ = new Data(*other.data_);
What if other == *this?
File src/kudu/common/schema.h:
PS28, Line 91:  public:
nit: it's public by default in struct, no need to specify explicitly
File src/kudu/integration-tests/
PS28, Line 52: const int kNumServers = 3;
nit: it's 3 tablet server by default, you could drop this.
PS28, Line 55: {}, {}, kNumServers
nit: I think it's possible to drop all this and use the parameters 'by default'
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 to 
READ_AT_SNAPSHOT 'automagically'.
PS28, Line 172:     }
nit: would it make sense to add an 'erroneous case' trying to read decimal as, 
say, float/double/string/ etc.?

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 19:25:00 +0000
Gerrit-HasComments: Yes

Reply via email to