You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by mp...@apache.org on 2019/02/28 01:13:21 UTC
[kudu] 01/04: Schema copy and move constructors/operators must copy
max_col_id_
This is an automated email from the ASF dual-hosted git repository.
mpercy pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit cfc7b9d45ada5b3f36be045ec0dfa81c16b2cc99
Author: Mike Percy <mp...@apache.org>
AuthorDate: Fri Feb 8 00:16:43 2019 -0800
Schema copy and move constructors/operators must copy max_col_id_
The CopyFrom() helper method used by the Schema copy constructor and
assignment operator, as well as the move constructor and move operator,
previously did not copy the value of max_col_id_, leaving it
uninitialized in the copy. This patch addresses that oversight.
This patch also updates the Schema::Equals() method to provide the
option (not used by default) to check that the column ids and max column
id match between the compared schemas, allowing us to provide coverage
by beefing up an existing unit test.
Change-Id: I61b8df1142dc733c446a5717ca8cae1309f3e867
Reviewed-on: http://gerrit.cloudera.org:8080/12415
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
src/kudu/common/schema-test.cc | 60 ++++++++++++++++++++++++++++--------------
src/kudu/common/schema.cc | 5 +++-
src/kudu/common/schema.h | 30 ++++++++++++++++-----
3 files changed, 68 insertions(+), 27 deletions(-)
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index 198bf00..a35e3be 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -121,7 +121,18 @@ TEST_F(TestSchema, TestSchemaToStringMode) {
schema.ToString(Schema::ToStringMode::BASE_INFO));
}
-TEST_F(TestSchema, TestCopyAndMove) {
+enum IncludeColumnIds {
+ INCLUDE_COL_IDS,
+ NO_COL_IDS
+};
+
+class ParameterizedSchemaTest : public KuduTest,
+ public ::testing::WithParamInterface<IncludeColumnIds> {};
+
+INSTANTIATE_TEST_CASE_P(SchemaTypes, ParameterizedSchemaTest,
+ ::testing::Values(INCLUDE_COL_IDS, NO_COL_IDS));
+
+TEST_P(ParameterizedSchemaTest, TestCopyAndMove) {
auto check_schema = [](const Schema& schema) {
ASSERT_EQ(sizeof(Slice) + sizeof(uint32_t) + sizeof(int32_t),
schema.byte_size());
@@ -129,12 +140,15 @@ TEST_F(TestSchema, TestCopyAndMove) {
ASSERT_EQ(0, schema.column_offset(0));
ASSERT_EQ(sizeof(Slice), schema.column_offset(1));
- EXPECT_EQ("(\n"
- " key STRING NOT NULL,\n"
- " uint32val UINT32 NULLABLE,\n"
- " int32val INT32 NOT NULL,\n"
- " PRIMARY KEY (key)\n"
- ")",
+ EXPECT_EQ(Substitute("(\n"
+ " $0key STRING NOT NULL,\n"
+ " $1uint32val UINT32 NULLABLE,\n"
+ " $2int32val INT32 NOT NULL,\n"
+ " PRIMARY KEY (key)\n"
+ ")",
+ schema.has_column_ids() ? "0:" : "",
+ schema.has_column_ids() ? "1:" : "",
+ schema.has_column_ids() ? "2:" : ""),
schema.ToString());
EXPECT_EQ("key STRING NOT NULL", schema.column(0).ToString());
EXPECT_EQ("UINT32 NULLABLE", schema.column(1).TypeToString());
@@ -145,38 +159,44 @@ TEST_F(TestSchema, TestCopyAndMove) {
ColumnSchema col3("int32val", INT32);
vector<ColumnSchema> cols = { col1, col2, col3 };
- Schema schema(cols, 1);
- check_schema(schema);
+ vector<ColumnId> ids = { ColumnId(0), ColumnId(1), ColumnId(2) };
+ const int kNumKeyCols = 1;
+
+ Schema schema = GetParam() == INCLUDE_COL_IDS ?
+ Schema(cols, ids, kNumKeyCols) :
+ Schema(cols, kNumKeyCols);
+
+ NO_FATALS(check_schema(schema));
// Check copy- and move-assignment.
Schema moved_schema;
{
Schema copied_schema = schema;
- check_schema(copied_schema);
- ASSERT_TRUE(copied_schema.Equals(schema));
+ NO_FATALS(check_schema(copied_schema));
+ ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL));
// Move-assign to 'moved_to_schema' from 'copied_schema' and then let
- // 'copied_schema' go out of scope to make sure none of 'moved_to_schema''s
+ // 'copied_schema' go out of scope to make sure none of the 'moved_schema'
// resources are incorrectly freed.
moved_schema = std::move(copied_schema);
// 'copied_schema' is moved from so it should still be valid to call
// ToString(), though we can't expect any particular result.
- NO_FATALS(copied_schema.ToString()); // NOLINT(*)
+ copied_schema.ToString(); // NOLINT(*)
}
- check_schema(moved_schema);
- ASSERT_TRUE(moved_schema.Equals(schema));
+ NO_FATALS(check_schema(moved_schema));
+ ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL));
// Check copy- and move-construction.
{
Schema copied_schema(schema);
- check_schema(copied_schema);
- ASSERT_TRUE(copied_schema.Equals(schema));
+ NO_FATALS(check_schema(copied_schema));
+ ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL));
Schema moved_schema(std::move(copied_schema));
- NO_FATALS(copied_schema.ToString()); // NOLINT(*)
- check_schema(moved_schema);
- ASSERT_TRUE(moved_schema.Equals(schema));
+ copied_schema.ToString(); // NOLINT(*)
+ NO_FATALS(check_schema(moved_schema));
+ ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL));
}
}
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index 46f74a1..3ce4fb9 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -170,6 +170,7 @@ void Schema::CopyFrom(const Schema& other) {
num_key_columns_ = other.num_key_columns_;
cols_ = other.cols_;
col_ids_ = other.col_ids_;
+ max_col_id_ = other.max_col_id_;
col_offsets_ = other.col_offsets_;
id_to_index_ = other.id_to_index_;
@@ -189,9 +190,10 @@ Schema::Schema(Schema&& other) noexcept
: cols_(std::move(other.cols_)),
num_key_columns_(other.num_key_columns_),
col_ids_(std::move(other.col_ids_)),
+ max_col_id_(other.max_col_id_),
col_offsets_(std::move(other.col_offsets_)),
name_to_index_bytes_(0),
- name_to_index_(/*bucket_count=*/10,
+ name_to_index_(/*bucket_count*/ 10,
NameToIndexMap::hasher(),
NameToIndexMap::key_equal(),
NameToIndexMapAllocator(&name_to_index_bytes_)),
@@ -215,6 +217,7 @@ Schema& Schema::operator=(Schema&& other) noexcept {
cols_ = std::move(other.cols_);
num_key_columns_ = other.num_key_columns_;
col_ids_ = std::move(other.col_ids_);
+ max_col_id_ = other.max_col_id_;
col_offsets_ = std::move(other.col_offsets_);
id_to_index_ = std::move(other.id_to_index_);
has_nullables_ = other.has_nullables_;
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index cd6dbc9..fd6b40e 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -747,15 +747,33 @@ class Schema {
// so should only be used when necessary for output.
std::string ToString(ToStringMode mode = ToStringMode::WITH_COLUMN_IDS) const;
+ // Compare column ids in Equals() method.
+ enum SchemaComparisonType {
+ COMPARE_COLUMNS = 1 << 0,
+ COMPARE_COLUMN_IDS = 1 << 1,
+
+ COMPARE_ALL = COMPARE_COLUMNS | COMPARE_COLUMN_IDS
+ };
+
// Return true if the schemas have exactly the same set of columns
// and respective types.
- bool Equals(const Schema &other) const {
+ bool Equals(const Schema& other, SchemaComparisonType flags = COMPARE_COLUMNS) const {
if (this == &other) return true;
- if (this->num_key_columns_ != other.num_key_columns_) return false;
- if (this->num_columns() != other.num_columns()) return false;
- for (size_t i = 0; i < other.num_columns(); i++) {
- if (!this->cols_[i].Equals(other.cols_[i])) return false;
+ if (flags & COMPARE_COLUMNS) {
+ if (this->num_key_columns_ != other.num_key_columns_) return false;
+ if (this->num_columns() != other.num_columns()) return false;
+ for (size_t i = 0; i < other.num_columns(); i++) {
+ if (!this->cols_[i].Equals(other.cols_[i])) return false;
+ }
+ }
+
+ if (flags & COMPARE_COLUMN_IDS) {
+ if (this->has_column_ids() != other.has_column_ids()) return false;
+ if (this->has_column_ids()) {
+ if (this->col_ids_ != other.col_ids_) return false;
+ if (this->max_col_id() != other.max_col_id()) return false;
+ }
}
return true;
@@ -918,8 +936,8 @@ class Schema {
std::vector<ColumnSchema> cols_;
size_t num_key_columns_;
- ColumnId max_col_id_;
std::vector<ColumnId> col_ids_;
+ ColumnId max_col_id_;
std::vector<size_t> col_offsets_;
// The keys of this map are StringPiece references to the actual name members of the