Skip to content

Commit

Permalink
Schema copy and move constructors/operators must copy max_col_id_
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@mpercy
mpercy committed Feb 28, 2019
1 parent 1aae078 commit cfc7b9d
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 27 deletions.
60 changes: 40 additions & 20 deletions src/kudu/common/schema-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,20 +121,34 @@ 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());
ASSERT_EQ(3, schema.num_columns());
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());
Expand All @@ -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));
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/kudu/common/schema.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;

Expand All @@ -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_)),
Expand All @@ -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_;
Expand Down
30 changes: 24 additions & 6 deletions src/kudu/common/schema.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit cfc7b9d

Please sign in to comment.