kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aw...@apache.org
Subject [1/4] kudu git commit: Remove column ids from KuduSchema::ToString
Date Tue, 16 Oct 2018 04:34:44 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 5894af6ff -> 1197eb01a


Remove column ids from KuduSchema::ToString

708d0f736c5a9d60ca02041bdb3b753e11478b6d added a KuduSchema::ToString
method, which calls into Schema::ToString. This is fine except
Schema::ToString includes the column ids when they are set, and that
information is implementation and ought not be shown to the user.

I think the original testing missed this because it never called
KuduSchema::ToString on a KuduSchema built from a Schema that had column
ids. A new test fixes this oversight.

Change-Id: Ib5999c485b63b6bdd4f4778b329f6768680b5193
Reviewed-on: http://gerrit.cloudera.org:8080/11668
Tested-by: Will Berkeley <wdberkeley@gmail.com>
Reviewed-by: Dan Burkert <danburkert@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/df5fc241
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/df5fc241
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/df5fc241

Branch: refs/heads/master
Commit: df5fc241e5e459a0978afe5035eca1a24f66d017
Parents: 5894af6
Author: Will Berkeley <wdberkeley@gmail.org>
Authored: Fri Oct 12 10:11:24 2018 -0700
Committer: Will Berkeley <wdberkeley@gmail.com>
Committed: Fri Oct 12 22:05:03 2018 +0000

----------------------------------------------------------------------
 src/kudu/client/client-unittest.cc | 26 +++++++++++++++++++++++++-
 src/kudu/client/schema.cc          |  3 ++-
 src/kudu/common/schema-test.cc     | 13 ++++++++++++-
 src/kudu/common/schema.cc          |  4 ++--
 src/kudu/common/schema.h           |  9 ++++++++-
 5 files changed, 49 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/df5fc241/src/kudu/client/client-unittest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-unittest.cc b/src/kudu/client/client-unittest.cc
index 3ac2cf9..d2c4866 100644
--- a/src/kudu/client/client-unittest.cc
+++ b/src/kudu/client/client-unittest.cc
@@ -17,6 +17,8 @@
 //
 // Tests for the client which are true unit tests and don't require a cluster, etc.
 
+#include "kudu/client/client.h"
+
 #include <cstddef>
 #include <string>
 #include <vector>
@@ -25,18 +27,22 @@
 #include <boost/function.hpp>
 #include <gtest/gtest.h>
 
-#include "kudu/client/client.h"
 #include "kudu/client/client-internal.h"
+#include "kudu/client/client-test-util.h"
 #include "kudu/client/error_collector.h"
 #include "kudu/client/schema.h"
 #include "kudu/client/value.h"
+#include "kudu/common/common.pb.h"
+#include "kudu/common/schema.h"
 #include "kudu/gutil/ref_counted.h"
+#include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/monotime.h"
 #include "kudu/util/status.h"
 #include "kudu/util/test_macros.h"
 
 using std::string;
 using std::vector;
+using strings::Substitute;
 using kudu::client::internal::ErrorCollector;
 
 namespace kudu {
@@ -272,5 +278,23 @@ TEST(ClientUnitTest, TestKuduSchemaToString) {
   EXPECT_EQ(schema_str_2, s2.ToString());
 }
 
+TEST(ClientUnitTest, TestKuduSchemaToStringWithColumnIds) {
+  // Build a KuduSchema from a Schema, so that the KuduSchema's internal Schema
+  // has column ids.
+  SchemaBuilder builder;
+  builder.AddKeyColumn("key", DataType::INT32);
+  const auto schema = builder.Build();
+  const auto kudu_schema = KuduSchemaFromSchema(schema);
+
+  // The string version of the KuduSchema should not have column ids, even
+  // though the default string version of the underlying Schema should.
+  EXPECT_EQ(
+      Substitute("Schema [\n\tprimary key (key),\n\t$0:key[int32 NOT NULL]\n]",
+                 schema.column_id(0)),
+      schema.ToString());
+  EXPECT_EQ("Schema [\n\tprimary key (key),\n\tkey[int32 NOT NULL]\n]",
+            kudu_schema.ToString());
+}
+
 } // namespace client
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/df5fc241/src/kudu/client/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc
index f250fd0..308e49d 100644
--- a/src/kudu/client/schema.cc
+++ b/src/kudu/client/schema.cc
@@ -738,7 +738,8 @@ void KuduSchema::GetPrimaryKeyColumnIndexes(vector<int>* indexes)
const {
 }
 
 string KuduSchema::ToString() const {
-  return schema_ ? schema_->ToString() : "Schema []";
+  return schema_ ? schema_->ToString(Schema::ToStringMode::WITHOUT_COLUMN_IDS)
+                 : "Schema []";
 }
 
 } // namespace client

http://git-wip-us.apache.org/repos/asf/kudu/blob/df5fc241/src/kudu/common/schema-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index 87ba59a..3fcbdbb 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -103,6 +103,18 @@ TEST_F(TestSchema, TestSchema) {
   EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString());
 }
 
+TEST_F(TestSchema, TestSchemaToStringMode) {
+  SchemaBuilder builder;
+  builder.AddKeyColumn("key", DataType::INT32);
+  const auto schema = builder.Build();
+  EXPECT_EQ(
+      Substitute("Schema [\n\tprimary key (key),\n\t$0:key[int32 NOT NULL]\n]",
+                 schema.column_id(0)),
+      schema.ToString());
+  EXPECT_EQ("Schema [\n\tprimary key (key),\n\tkey[int32 NOT NULL]\n]",
+            schema.ToString(Schema::ToStringMode::WITHOUT_COLUMN_IDS));
+}
+
 TEST_F(TestSchema, TestCopyAndMove) {
   auto check_schema = [](const Schema& schema) {
     ASSERT_EQ(sizeof(Slice) + sizeof(uint32_t) + sizeof(int32_t),
@@ -403,7 +415,6 @@ TEST_F(TestSchema, TestProjectRename) {
   ASSERT_EQ(row_projector.projection_defaults()[0], 2);      // non_present schema2
 }
 
-
 // Test that the schema can be used to compare and stringify rows.
 TEST_F(TestSchema, TestRowOperations) {
   Schema schema({ ColumnSchema("col1", STRING),

http://git-wip-us.apache.org/repos/asf/kudu/blob/df5fc241/src/kudu/common/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index b15652e..dbd9cc7 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -388,7 +388,7 @@ Status Schema::GetMappedReadProjection(const Schema& projection,
   return Status::OK();
 }
 
-string Schema::ToString() const {
+string Schema::ToString(ToStringMode mode) const {
   if (cols_.empty()) return "Schema []";
 
   vector<string> pk_strs;
@@ -397,7 +397,7 @@ string Schema::ToString() const {
   }
 
   vector<string> col_strs;
-  if (has_column_ids()) {
+  if (has_column_ids() && mode != ToStringMode::WITHOUT_COLUMN_IDS) {
     for (int i = 0; i < cols_.size(); ++i) {
       col_strs.push_back(Substitute("$0:$1", col_ids_[i], cols_[i].ToString()));
     }

http://git-wip-us.apache.org/repos/asf/kudu/blob/df5fc241/src/kudu/common/schema.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index 449f3cd..64283f0 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -723,9 +723,16 @@ class Schema {
     return Slice(*dst);
   }
 
+  // Enum to configure how a Schema is stringified.
+  enum class ToStringMode {
+    // Include column ids if this instance has them.
+    WITH_COLUMN_IDS,
+    // Do not include column ids.
+    WITHOUT_COLUMN_IDS,
+  };
   // Stringify this Schema. This is not particularly efficient,
   // so should only be used when necessary for output.
-  std::string ToString() const;
+  std::string ToString(ToStringMode mode = ToStringMode::WITH_COLUMN_IDS) const;
 
   // Return true if the schemas have exactly the same set of columns
   // and respective types.


Mime
View raw message