kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] 03/03: KUDU-1711: ColumnSchema supports storing column comment
Date Fri, 29 Mar 2019 04:18:48 GMT
This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 62d1dec5d5a5c31647d3a92ff203e103198a92d1
Author: helifu <hzhelifu@corp.netease.com>
AuthorDate: Tue Mar 26 10:51:05 2019 +0800

    KUDU-1711: ColumnSchema supports storing column comment
    
    This patch intends to support storing column comment in ColumnSchema,
    and the comment is displayed on the 'master:port/table?id=uuid' and
    'tserver:port/tablet?id=uuid'.
    
    Note: in this patch c++ client has been added 'Comment()' API, but
    other clients are not yet.
    
    Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990
    Reviewed-on: http://gerrit.cloudera.org:8080/12849
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <adar@cloudera.com>
---
 src/kudu/client/batcher.cc                |   4 +-
 src/kudu/client/client-test.cc            | 112 ++++++++++++++++++++++++++++++
 src/kudu/client/schema-internal.h         |   4 ++
 src/kudu/client/schema.cc                 |  20 ++++--
 src/kudu/client/schema.h                  |  18 ++++-
 src/kudu/client/table_alterer-internal.cc |  10 ++-
 src/kudu/common/common.proto              |   5 ++
 src/kudu/common/schema-test.cc            |   6 +-
 src/kudu/common/schema.cc                 |   3 +
 src/kudu/common/schema.h                  |  33 +++++++--
 src/kudu/common/wire_protocol.cc          |  18 ++++-
 src/kudu/common/wire_protocol.h           |   2 +
 src/kudu/master/catalog_manager.cc        |  37 +++++++---
 src/kudu/server/webui_util.cc             |   3 +
 www/table.mustache                        |   2 +
 www/tablet.mustache                       |   2 +
 16 files changed, 253 insertions(+), 26 deletions(-)

diff --git a/src/kudu/client/batcher.cc b/src/kudu/client/batcher.cc
index 716a1ec..e6ce9dd 100644
--- a/src/kudu/client/batcher.cc
+++ b/src/kudu/client/batcher.cc
@@ -304,7 +304,9 @@ WriteRpc::WriteRpc(const scoped_refptr<Batcher>& batcher,
 
   // Set up schema
   CHECK_OK(SchemaToPB(*schema, req_.mutable_schema(),
-                      SCHEMA_PB_WITHOUT_STORAGE_ATTRIBUTES | SCHEMA_PB_WITHOUT_IDS));
+                      SCHEMA_PB_WITHOUT_STORAGE_ATTRIBUTES |
+                      SCHEMA_PB_WITHOUT_IDS |
+                      SCHEMA_PB_WITHOUT_COMMENT));
 
   // Pick up the authz token for the table.
   FetchCachedAuthzToken();
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 44d6f2a..a75fd6d 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -127,6 +127,7 @@ DECLARE_int32(leader_failure_exp_backoff_max_delta_ms);
 DECLARE_int32(log_inject_latency_ms_mean);
 DECLARE_int32(log_inject_latency_ms_stddev);
 DECLARE_int32(master_inject_latency_on_tablet_lookups_ms);
+DECLARE_int32(max_column_comment_length);
 DECLARE_int32(max_create_tablets_per_ts);
 DECLARE_int32(raft_heartbeat_interval_ms);
 DECLARE_int32(scanner_batch_size_rows);
@@ -4886,6 +4887,117 @@ TEST_F(ClientTest, TestReadAtSnapshotNoTimestampSet) {
   EXPECT_EQ(kTabletsNum * kRowsPerTablet, total_row_count);
 }
 
+TEST_F(ClientTest, TestCreateTableWithValidComment) {
+  const string kTableName = "table_comment";
+  const string kLongComment(FLAGS_max_column_comment_length, 'x');
+
+  // Create a table with comment.
+  {
+    KuduSchema schema;
+    KuduSchemaBuilder schema_builder;
+    schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+    schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32)->Comment(kLongComment);
+    ASSERT_OK(schema_builder.Build(&schema));
+    unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+    ASSERT_OK(table_creator->table_name(kTableName)
+      .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" }).Create());
+  }
+
+  // Open the table and verify the comment.
+  {
+    KuduSchema schema;
+    ASSERT_OK(client_->GetTableSchema(kTableName, &schema));
+    ASSERT_EQ(schema.Column(1).name(), "val");
+    ASSERT_EQ(schema.Column(1).comment(), kLongComment);
+  }
+}
+
+TEST_F(ClientTest, TestAlterTableWithValidComment) {
+  const string kTableName = "table_comment";
+  const auto AlterAndVerify = [&] (int i) {
+    // The comment length should be less and equal than FLAGS_max_column_comment_length.
+    string kLongComment(i * 16, 'x');
+
+    // Alter the table with comment.
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("val")->Comment(kLongComment);
+    ASSERT_OK(table_alterer->Alter());
+
+    // Open the table and verify the comment.
+    KuduSchema schema;
+    ASSERT_OK(client_->GetTableSchema(kTableName, &schema));
+    ASSERT_EQ(schema.Column(1).name(), "val");
+    ASSERT_EQ(schema.Column(1).comment(), kLongComment);
+  };
+
+  // Create a table.
+  {
+    KuduSchema schema;
+    KuduSchemaBuilder schema_builder;
+    schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+    schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32);
+    ASSERT_OK(schema_builder.Build(&schema));
+    unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+    ASSERT_OK(table_creator->table_name(kTableName)
+        .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" }).Create());
+  }
+
+  for (int i = 0; i <= 16; ++i) {
+    NO_FATALS(AlterAndVerify(i));
+  }
+}
+
+TEST_F(ClientTest, TestCreateAndAlterTableWithInvalidComment) {
+  const string kTableName = "table_comment";
+  const vector<pair<string, string>> kCases = {
+    {string(FLAGS_max_column_comment_length + 1, 'x'), "longer than maximum permitted length"},
+    {string("foo\0bar", 7), "invalid column comment: identifier must not contain null bytes"},
+    {string("foo\xf0\x28\x8c\xbc", 7), "invalid column comment: invalid UTF8 sequence"}
+  };
+
+  // Create tables with invalid comment.
+  for (const auto& test_case : kCases) {
+    const auto& comment = test_case.first;
+    const auto& substr = test_case.second;
+    SCOPED_TRACE(comment);
+
+    KuduSchema schema;
+    KuduSchemaBuilder schema_builder;
+    schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+    schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32)->Comment(comment);
+    ASSERT_OK(schema_builder.Build(&schema));
+    unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+    Status s = table_creator->table_name(kTableName)
+        .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" }).Create();
+    ASSERT_STR_CONTAINS(s.ToString(), substr);
+  }
+
+  // Create a table for later alterer.
+  {
+    KuduSchema schema;
+    KuduSchemaBuilder schema_builder;
+    schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+    schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32);
+    ASSERT_OK(schema_builder.Build(&schema));
+    unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+    ASSERT_OK(table_creator->table_name(kTableName)
+        .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" }).Create());
+  }
+
+  // Alter tables with invalid comment.
+  for (const auto& test_case : kCases) {
+    const auto& comment = test_case.first;
+    const auto& substr = test_case.second;
+    SCOPED_TRACE(comment);
+
+    // Alter the table.
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("val")->Comment(comment);
+    Status s = table_alterer->Alter();
+    ASSERT_STR_CONTAINS(s.ToString(), substr);
+  }
+}
+
 enum IntEncoding {
   kPlain,
   kBitShuffle,
diff --git a/src/kudu/client/schema-internal.h b/src/kudu/client/schema-internal.h
index 10f6132..923ea35 100644
--- a/src/kudu/client/schema-internal.h
+++ b/src/kudu/client/schema-internal.h
@@ -19,6 +19,8 @@
 
 #include <string>
 
+#include <boost/optional/optional.hpp>
+
 #include "kudu/client/schema.h"
 #include "kudu/client/value.h"
 #include "kudu/common/common.pb.h"
@@ -112,6 +114,8 @@ class KuduColumnSpec::Data {
   // For ALTER
   bool has_rename_to;
   std::string rename_to;
+
+  boost::optional<std::string> comment;
 };
 
 } // namespace client
diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc
index 3ce5a1b..33bbab7 100644
--- a/src/kudu/client/schema.cc
+++ b/src/kudu/client/schema.cc
@@ -289,6 +289,11 @@ KuduColumnSpec* KuduColumnSpec::RenameTo(const std::string& new_name)
{
   return this;
 }
 
+KuduColumnSpec* KuduColumnSpec::Comment(const string& comment) {
+  data_->comment = boost::optional<string>(comment);
+  return this;
+}
+
 Status KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col) const {
   // Verify that the user isn't trying to use any methods that
   // don't make sense for CREATE.
@@ -377,7 +382,7 @@ Status KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col) const {
   *col = KuduColumnSchema(data_->name, data_->type, nullable,
                           default_val,
                           KuduColumnStorageAttributes(encoding, compression, block_size),
-                          type_attrs);
+                          type_attrs, data_->comment.get_ptr());
 #pragma GCC diagnostic pop
 
   return Status::OK();
@@ -424,6 +429,7 @@ Status KuduColumnSpec::ToColumnSchemaDelta(ColumnSchemaDelta* col_delta)
const {
     col_delta->cfile_block_size = boost::optional<int32_t>(data_->block_size);
   }
 
+  col_delta->new_comment = std::move(data_->comment);
   return Status::OK();
 }
 
@@ -602,7 +608,8 @@ KuduColumnSchema::KuduColumnSchema(const std::string &name,
                                    bool is_nullable,
                                    const void* default_value,
                                    const KuduColumnStorageAttributes& storage_attributes,
-                                   const KuduColumnTypeAttributes& type_attributes) {
+                                   const KuduColumnTypeAttributes& type_attributes,
+                                   const string* comment) {
   ColumnStorageAttributes attr_private;
   attr_private.encoding = ToInternalEncodingType(storage_attributes.encoding());
   attr_private.compression = ToInternalCompressionType(
@@ -613,7 +620,8 @@ KuduColumnSchema::KuduColumnSchema(const std::string &name,
   col_ = new ColumnSchema(name, ToInternalDataType(type, type_attributes),
                           is_nullable,
                           default_value, default_value, attr_private,
-                          type_attr_private);
+                          type_attr_private,
+                          std::move(comment ? boost::optional<string>(*comment) : boost::none));
 }
 
 KuduColumnSchema::KuduColumnSchema(const KuduColumnSchema& other)
@@ -667,6 +675,10 @@ KuduColumnTypeAttributes KuduColumnSchema::type_attributes() const {
   return KuduColumnTypeAttributes(type_attributes.precision, type_attributes.scale);
 }
 
+string KuduColumnSchema::comment() const {
+  return DCHECK_NOTNULL(col_)->comment() ? *col_->comment() : "";
+}
+
 ////////////////////////////////////////////////////////////
 // KuduSchema
 ////////////////////////////////////////////////////////////
@@ -728,7 +740,7 @@ KuduColumnSchema KuduSchema::Column(size_t idx) const {
   KuduColumnTypeAttributes type_attrs(col.type_attributes().precision, col.type_attributes().scale);
   return KuduColumnSchema(col.name(), FromInternalDataType(col.type_info()->type()),
                           col.is_nullable(), col.read_default_value(),
-                          attrs, type_attrs);
+                          attrs, type_attrs, col.comment().get_ptr());
 }
 
 KuduPartialRow* KuduSchema::NewRow() const {
diff --git a/src/kudu/client/schema.h b/src/kudu/client/schema.h
index 5495ab7..b59fa39 100644
--- a/src/kudu/client/schema.h
+++ b/src/kudu/client/schema.h
@@ -28,6 +28,7 @@
 
 #ifdef KUDU_HEADERS_NO_STUBS
 #include <gtest/gtest_prod.h>
+
 #include "kudu/gutil/port.h"
 #else
 #include "kudu/client/stubs.h"
@@ -241,6 +242,12 @@ class KUDU_EXPORT KuduColumnSchema {
   /// @return Type attributes of the column schema.
   KuduColumnTypeAttributes type_attributes() const;
 
+  /// @return comment of the column schema.
+  ///
+  /// @note Both columns with no comments and empty comments will return
+  ///   empty strings here.
+  std::string comment() const;
+
  private:
   friend class KuduColumnSpec;
   friend class KuduSchema;
@@ -267,7 +274,9 @@ class KUDU_EXPORT KuduColumnSchema {
       bool is_nullable = false,
       const void* default_value = NULL, //NOLINT(modernize-use-nullptr)
       const KuduColumnStorageAttributes& storage_attributes = KuduColumnStorageAttributes(),
-      const KuduColumnTypeAttributes& type_attributes = KuduColumnTypeAttributes());
+      const KuduColumnTypeAttributes& type_attributes = KuduColumnTypeAttributes(),
+      const std::string* comment = NULL //NOLINT(modernize-use-nullptr)
+      );
 #if defined(__clang__) || \
   (defined(__GNUC__) && (__GNUC__ * 10000 + __GNUC_MINOR__ * 100) >= 40600)
 #pragma GCC diagnostic pop
@@ -432,6 +441,13 @@ class KUDU_EXPORT KuduColumnSpec {
   KuduColumnSpec* RenameTo(const std::string& new_name);
   ///@}
 
+  /// Set the comment of the column.
+  ///
+  /// @param [in] comment
+  ///   The comment for the column.
+  /// @return Pointer to the modified object.
+  KuduColumnSpec* Comment(const std::string& comment);
+
  private:
   class KUDU_NO_EXPORT Data;
   friend class KuduSchemaBuilder;
diff --git a/src/kudu/client/table_alterer-internal.cc b/src/kudu/client/table_alterer-internal.cc
index 650d0e0..4172ec3 100644
--- a/src/kudu/client/table_alterer-internal.cc
+++ b/src/kudu/client/table_alterer-internal.cc
@@ -69,7 +69,9 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) {
 
   if (schema_ != nullptr) {
     RETURN_NOT_OK(SchemaToPB(*schema_, req->mutable_schema(),
-                             SCHEMA_PB_WITHOUT_IDS | SCHEMA_PB_WITHOUT_WRITE_DEFAULT));
+                             SCHEMA_PB_WITHOUT_IDS |
+                             SCHEMA_PB_WITHOUT_WRITE_DEFAULT |
+                             SCHEMA_PB_WITHOUT_COMMENT));
   }
 
   for (const Step& s : steps_) {
@@ -105,7 +107,8 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) {
             !s.spec->data_->remove_default &&
             !s.spec->data_->has_encoding &&
             !s.spec->data_->has_compression &&
-            !s.spec->data_->has_block_size) {
+            !s.spec->data_->has_block_size &&
+            !s.spec->data_->comment) {
           return Status::InvalidArgument("no alter operation specified",
                                          s.spec->data_->name);
         }
@@ -117,7 +120,8 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) {
             !s.spec->data_->remove_default &&
             !s.spec->data_->has_encoding &&
             !s.spec->data_->has_compression &&
-            !s.spec->data_->has_block_size) {
+            !s.spec->data_->has_block_size &&
+            !s.spec->data_->comment) {
           pb_step->set_type(AlterTableRequestPB::RENAME_COLUMN);
           pb_step->mutable_rename_column()->set_old_name(s.spec->data_->name);
           pb_step->mutable_rename_column()->set_new_name(s.spec->data_->rename_to);
diff --git a/src/kudu/common/common.proto b/src/kudu/common/common.proto
index ceb281d..a51e806 100644
--- a/src/kudu/common/common.proto
+++ b/src/kudu/common/common.proto
@@ -116,6 +116,9 @@ message ColumnSchemaPB {
   optional int32 cfile_block_size = 10 [default=0];
 
   optional ColumnTypeAttributesPB type_attributes = 11;
+
+  // The comment for the column.
+  optional string comment = 12;
 }
 
 message ColumnSchemaDeltaPB {
@@ -128,6 +131,8 @@ message ColumnSchemaDeltaPB {
   optional EncodingType encoding = 6;
   optional CompressionType compression = 7;
   optional int32 block_size = 8;
+
+  optional string new_comment = 9;
 }
 
 message SchemaPB {
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index ee428f7..1c5e8c6 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -273,9 +273,9 @@ TEST_F(TestSchema, TestColumnSchemaEquals) {
   ASSERT_TRUE(col1.Equals(col2, ColumnSchema::COMPARE_TYPE));
   ASSERT_TRUE(col1.Equals(col3, ColumnSchema::COMPARE_NAME));
   ASSERT_FALSE(col1.Equals(col3, ColumnSchema::COMPARE_TYPE));
-  ASSERT_TRUE(col1.Equals(col3, ColumnSchema::COMPARE_DEFAULTS));
-  ASSERT_FALSE(col3.Equals(col4, ColumnSchema::COMPARE_DEFAULTS));
-  ASSERT_TRUE(col4.Equals(col4, ColumnSchema::COMPARE_DEFAULTS));
+  ASSERT_TRUE(col1.Equals(col3, ColumnSchema::COMPARE_OTHER));
+  ASSERT_FALSE(col3.Equals(col4, ColumnSchema::COMPARE_OTHER));
+  ASSERT_TRUE(col4.Equals(col4, ColumnSchema::COMPARE_OTHER));
 }
 
 TEST_F(TestSchema, TestSchemaEquals) {
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index 3cb3c98..81ba0a2 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -115,6 +115,9 @@ Status ColumnSchema::ApplyDelta(const ColumnSchemaDelta& col_delta)
{
   if (col_delta.cfile_block_size) {
     attributes_.cfile_block_size = *col_delta.cfile_block_size;
   }
+  if (col_delta.new_comment) {
+    comment_ = col_delta.new_comment;
+  }
   return Status::OK();
 }
 
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index bc4cefd..89dbdd9 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -174,6 +174,8 @@ public:
   boost::optional<EncodingType> encoding;
   boost::optional<CompressionType> compression;
   boost::optional<int32_t> cfile_block_size;
+
+  boost::optional<std::string> new_comment;
 };
 
 // The schema for a given column.
@@ -190,6 +192,7 @@ class ColumnSchema {
   // write_default: default value added to the row if the column value was
   //    not specified on insert.
   //    The value will be copied and released on ColumnSchema destruction.
+  // comment: the comment for the column.
   //
   // Example:
   //   ColumnSchema col_a("a", UINT32)
@@ -202,13 +205,15 @@ class ColumnSchema {
                const void* read_default = nullptr,
                const void* write_default = nullptr,
                ColumnStorageAttributes attributes = ColumnStorageAttributes(),
-               ColumnTypeAttributes type_attributes = ColumnTypeAttributes())
+               ColumnTypeAttributes type_attributes = ColumnTypeAttributes(),
+               boost::optional<std::string> comment = boost::none)
       : name_(std::move(name)),
         type_info_(GetTypeInfo(type)),
         is_nullable_(is_nullable),
         read_default_(read_default ? new Variant(type, read_default) : nullptr),
         attributes_(attributes),
-        type_attributes_(type_attributes) {
+        type_attributes_(type_attributes),
+        comment_(std::move(comment)) {
     if (write_default == read_default) {
       write_default_ = read_default_;
     } else if (write_default != nullptr) {
@@ -248,6 +253,10 @@ class ColumnSchema {
   // For example, "AUTO_ENCODING ZLIB 123 123".
   std::string AttrToString() const;
 
+  const boost::optional<std::string>& comment() const {
+    return comment_;
+  }
+
   // Returns true if the column has a read default value
   bool has_read_default() const {
     return read_default_ != nullptr;
@@ -303,10 +312,10 @@ class ColumnSchema {
   enum CompareFlags {
     COMPARE_NAME = 1 << 0,
     COMPARE_TYPE = 1 << 1,
-    COMPARE_DEFAULTS = 1 << 2,
+    COMPARE_OTHER = 1 << 2,
 
     COMPARE_NAME_AND_TYPE = COMPARE_NAME | COMPARE_TYPE,
-    COMPARE_ALL = COMPARE_NAME | COMPARE_TYPE | COMPARE_DEFAULTS
+    COMPARE_ALL = COMPARE_NAME | COMPARE_TYPE | COMPARE_OTHER
   };
 
   bool Equals(const ColumnSchema &other,
@@ -322,7 +331,7 @@ class ColumnSchema {
     // For Key comparison checking the defaults doesn't make sense,
     // since we don't support them, for server vs user schema this comparison
     // will always fail, since the user does not specify the defaults.
-    if (flags & COMPARE_DEFAULTS) {
+    if (flags & COMPARE_OTHER) {
       if (read_default_ == nullptr && other.read_default_ != nullptr)
         return false;
 
@@ -334,6 +343,19 @@ class ColumnSchema {
 
       if (write_default_ != nullptr && !write_default_->Equals(other.write_default_.get()))
         return false;
+
+      // "no comment" and "empty comment" are the same.
+      if (comment_) {
+        if (other.comment_) {
+          if (*comment_ != *other.comment_) return false;
+        } else {
+          if (!comment_->empty()) return false;
+        }
+      } else {
+        if (other.comment_) {
+          if (!other.comment_->empty()) return false;
+        }
+      }
     }
     return true;
   }
@@ -405,6 +427,7 @@ class ColumnSchema {
   std::shared_ptr<Variant> write_default_;
   ColumnStorageAttributes attributes_;
   ColumnTypeAttributes type_attributes_;
+  boost::optional<std::string> comment_;
 };
 
 // The schema for a set of rows.
diff --git a/src/kudu/common/wire_protocol.cc b/src/kudu/common/wire_protocol.cc
index dc7b1f2..fa30e15 100644
--- a/src/kudu/common/wire_protocol.cc
+++ b/src/kudu/common/wire_protocol.cc
@@ -23,6 +23,7 @@
 #include <cstring>
 #include <ostream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <boost/optional/optional.hpp>
@@ -246,6 +247,9 @@ void ColumnSchemaToPB(const ColumnSchema& col_schema, ColumnSchemaPB
*pb, int fl
       pb->set_write_default_value(write_value, col_schema.type_info()->size());
     }
   }
+  if (col_schema.comment() && !(flags & SCHEMA_PB_WITHOUT_COMMENT)) {
+    pb->set_comment(*col_schema.comment());
+  }
 }
 
 ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
@@ -292,9 +296,15 @@ ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
   if (pb.has_cfile_block_size()) {
     attributes.cfile_block_size = pb.cfile_block_size();
   }
+
+  boost::optional<string> comment;
+  if (pb.has_comment()) {
+    comment = boost::optional<string>(pb.comment());
+  }
+
   return ColumnSchema(pb.name(), pb.type(), pb.is_nullable(),
                       read_default_ptr, write_default_ptr,
-                      attributes, type_attributes);
+                      attributes, type_attributes, std::move(comment));
 }
 
 void ColumnSchemaDeltaToPB(const ColumnSchemaDelta& col_delta, ColumnSchemaDeltaPB *pb)
{
@@ -319,6 +329,9 @@ void ColumnSchemaDeltaToPB(const ColumnSchemaDelta& col_delta, ColumnSchemaDelta
   if (col_delta.cfile_block_size) {
     pb->set_block_size(*col_delta.cfile_block_size);
   }
+  if (col_delta.new_comment) {
+    pb->set_new_comment(*col_delta.new_comment);
+  }
 }
 
 ColumnSchemaDelta ColumnSchemaDeltaFromPB(const ColumnSchemaDeltaPB& pb) {
@@ -341,6 +354,9 @@ ColumnSchemaDelta ColumnSchemaDeltaFromPB(const ColumnSchemaDeltaPB&
pb) {
   if (pb.has_block_size()) {
     col_delta.cfile_block_size = boost::optional<int32_t>(pb.block_size());
   }
+  if (pb.has_new_comment()) {
+    col_delta.new_comment = boost::optional<string>(pb.new_comment());
+  }
   return col_delta;
 }
 
diff --git a/src/kudu/common/wire_protocol.h b/src/kudu/common/wire_protocol.h
index cf6be92..3cb1883 100644
--- a/src/kudu/common/wire_protocol.h
+++ b/src/kudu/common/wire_protocol.h
@@ -92,6 +92,8 @@ enum SchemaPBConversionFlags {
   // protobuf. Used when sending schemas from the client to the master
   // for create/alter table.
   SCHEMA_PB_WITHOUT_WRITE_DEFAULT = 1 << 2,
+
+  SCHEMA_PB_WITHOUT_COMMENT = 1 << 3,
 };
 
 // Convert the specified schema to protobuf.
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index ed5d583..3a2f9b2 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -180,6 +180,10 @@ TAG_FLAG(max_num_columns, unsafe);
 
 DEFINE_int32(max_identifier_length, 256,
              "Maximum length of the name of a column or table.");
+
+DEFINE_int32(max_column_comment_length, 256,
+             "Maximum length of the comment of a column");
+
 // Tag as unsafe because we end up writing schemas in every WAL entry, etc,
 // and having very long column names would enter untested territory and affect
 // performance.
@@ -1297,19 +1301,15 @@ Status CatalogManager::CheckOnline() const {
 
 namespace {
 
-// Validate a table or column name to ensure that it is a valid identifier.
-Status ValidateIdentifier(const string& id) {
-  if (id.empty()) {
-    return Status::InvalidArgument("empty string not a valid identifier");
-  }
-
-  if (id.length() > FLAGS_max_identifier_length) {
+Status ValidateLengthAndUTF8(const string& id, int32_t max_length) {
+  // Id should not exceed the maximum allowed length.
+  if (id.length() > max_length) {
     return Status::InvalidArgument(Substitute(
         "identifier '$0' longer than maximum permitted length $1",
         id, FLAGS_max_identifier_length));
   }
 
-  // Identifiers should be valid UTF8.
+  // Id should be valid UTF8.
   const char* p = id.data();
   int rem = id.size();
   while (rem > 0) {
@@ -1328,6 +1328,25 @@ Status ValidateIdentifier(const string& id) {
   return Status::OK();
 }
 
+// Validate a table or column name to ensure that it is a valid identifier.
+Status ValidateIdentifier(const string& id) {
+  if (id.empty()) {
+    return Status::InvalidArgument("empty string not a valid identifier");
+  }
+
+  return ValidateLengthAndUTF8(id, FLAGS_max_identifier_length);
+}
+
+// Validate a column comment to ensure that it is a valid identifier.
+Status ValidateCommentIdentifier(const boost::optional<string>& id) {
+  // It's safe here because logical OR has left-to-right associativity.
+  if (!id || id->empty()) {
+    return Status::OK();
+  }
+
+  return ValidateLengthAndUTF8(*id, FLAGS_max_column_comment_length);
+}
+
 // Validate the client-provided schema and name.
 Status ValidateClientSchema(const optional<string>& name,
                             const Schema& schema) {
@@ -1337,6 +1356,8 @@ Status ValidateClientSchema(const optional<string>& name,
   for (int i = 0; i < schema.num_columns(); i++) {
     RETURN_NOT_OK_PREPEND(ValidateIdentifier(schema.column(i).name()),
                           "invalid column name");
+    RETURN_NOT_OK_PREPEND(ValidateCommentIdentifier(schema.column(i).comment()),
+                          "invalid column comment");
   }
   if (schema.num_key_columns() <= 0) {
     return Status::InvalidArgument("must specify at least one key column");
diff --git a/src/kudu/server/webui_util.cc b/src/kudu/server/webui_util.cc
index 590080f..44a45be 100644
--- a/src/kudu/server/webui_util.cc
+++ b/src/kudu/server/webui_util.cc
@@ -19,6 +19,8 @@
 
 #include <string>
 
+#include <boost/optional/optional.hpp>
+
 #include "kudu/common/common.pb.h"
 #include "kudu/common/schema.h"
 #include "kudu/gutil/ref_counted.h"
@@ -50,6 +52,7 @@ void SchemaToJson(const Schema& schema, EasyJson* output) {
                                    col.Stringify(col.read_default_value()) : "-";
     col_json["write_default"] = col.has_write_default() ?
                                     col.Stringify(col.write_default_value()) : "-";
+    col_json["comment"] = col.comment() ? *col.comment() : "-";
   }
 }
 
diff --git a/www/table.mustache b/www/table.mustache
index 8af9ebd..6a960d9 100644
--- a/www/table.mustache
+++ b/www/table.mustache
@@ -44,6 +44,7 @@ under the License.
       <th>Compression</th>
       <th>Read default</th>
       <th>Write default</th>
+      <th>Comment</th>
     </tr></thead>
     <tbody>
     {{#columns}}
@@ -57,6 +58,7 @@ under the License.
         <td>{{compression}}</td>
         <td>{{read_default}}</td>
         <td>{{write_default}}</td>
+        <td>{{comment}}</td>
       </tr>
     {{/columns}}
     </tbody>
diff --git a/www/tablet.mustache b/www/tablet.mustache
index aa83558..cbc0ff5 100644
--- a/www/tablet.mustache
+++ b/www/tablet.mustache
@@ -38,6 +38,7 @@ under the License.
       <th>Compression</th>
       <th>Read default</th>
       <th>Write default</th>
+      <th>Comment</th>
     </tr></thead>
     <tbody>
     {{#columns}}
@@ -51,6 +52,7 @@ under the License.
         <td>{{compression}}</td>
         <td>{{read_default}}</td>
         <td>{{write_default}}</td>
+        <td>{{comment}}</td>
       </tr>
     {{/columns}}
     </tbody>


Mime
View raw message