You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by wd...@apache.org on 2018/10/05 18:39:51 UTC
kudu git commit: Move constructor and assignment operator for Schema
Repository: kudu
Updated Branches:
refs/heads/master cf1b1f42c -> b848488d9
Move constructor and assignment operator for Schema
This adds a move constructor and move assignment operator to Schema.
I removed Schema::swap. It was only used in tests, and any potential
use of it should be adequately fulfilled by one of moves, copies, or
Schema::Reset.
Change-Id: I6c827c84657875be4cf2bc565db03a686849d8e2
Reviewed-on: http://gerrit.cloudera.org:8080/11593
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b848488d
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b848488d
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b848488d
Branch: refs/heads/master
Commit: b848488d9bd2efcfc63ddc073c3addece33793b6
Parents: cf1b1f4
Author: Will Berkeley <wd...@gmail.org>
Authored: Sun Sep 30 10:17:21 2018 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Fri Oct 5 18:38:54 2018 +0000
----------------------------------------------------------------------
src/kudu/common/id_mapping.h | 13 ------
src/kudu/common/schema-test.cc | 80 ++++++++++++++++++++++++++++---------
src/kudu/common/schema.cc | 49 ++++++++++++++++++-----
src/kudu/common/schema.h | 29 +++++++-------
4 files changed, 115 insertions(+), 56 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/id_mapping.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/id_mapping.h b/src/kudu/common/id_mapping.h
index a5d8289..f762b75 100644
--- a/src/kudu/common/id_mapping.h
+++ b/src/kudu/common/id_mapping.h
@@ -64,13 +64,6 @@ class IdMapping {
clear();
}
- explicit IdMapping(const IdMapping& other)
- : mask_(other.mask_),
- entries_(other.entries_) {
- }
-
- ~IdMapping() {}
-
void clear() {
ClearMap(&entries_);
}
@@ -84,12 +77,6 @@ class IdMapping {
other.entries_.swap(entries_);
}
- IdMapping& operator=(const IdMapping& other) {
- mask_ = other.mask_;
- entries_ = other.entries_;
- return *this;
- }
-
int operator[](int key) const {
return get(key);
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index 503ebea..87ba59a 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -23,6 +23,7 @@
#include <string>
#include <tuple> // IWYU pragma: keep
#include <unordered_map>
+#include <utility>
#include <vector>
#include <glog/logging.h> // IWYU pragma: keep
@@ -102,6 +103,65 @@ TEST_F(TestSchema, TestSchema) {
EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString());
}
+TEST_F(TestSchema, 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("Schema [\n"
+ "\tprimary key (key),\n"
+ "\tkey[string NOT NULL],\n"
+ "\tuint32val[uint32 NULLABLE],\n"
+ "\tint32val[int32 NOT NULL]\n"
+ "]",
+ schema.ToString());
+ EXPECT_EQ("key[string NOT NULL]", schema.column(0).ToString());
+ EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString());
+ };
+
+ ColumnSchema col1("key", STRING);
+ ColumnSchema col2("uint32val", UINT32, true);
+ ColumnSchema col3("int32val", INT32);
+
+ vector<ColumnSchema> cols = { col1, col2, col3 };
+ Schema schema(cols, 1);
+ 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));
+
+ // 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
+ // 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(*)
+ }
+ check_schema(moved_schema);
+ ASSERT_TRUE(moved_schema.Equals(schema));
+
+ // Check copy- and move-construction.
+ {
+ Schema copied_schema(schema);
+ check_schema(copied_schema);
+ ASSERT_TRUE(copied_schema.Equals(schema));
+
+ Schema moved_schema(std::move(copied_schema));
+ NO_FATALS(copied_schema.ToString()); // NOLINT(*)
+ check_schema(moved_schema);
+ ASSERT_TRUE(moved_schema.Equals(schema));
+ }
+}
+
// Test basic functionality of Schema definition with decimal columns
TEST_F(TestSchema, TestSchemaWithDecimal) {
ColumnSchema col1("key", STRING);
@@ -163,21 +223,6 @@ TEST_F(TestSchema, TestSchemaEqualsWithDecimal) {
EXPECT_FALSE(schema_18_10.Equals(schema_17_9));
}
-TEST_F(TestSchema, TestSwap) {
- Schema schema1({ ColumnSchema("col1", STRING),
- ColumnSchema("col2", STRING),
- ColumnSchema("col3", UINT32) },
- 2);
- Schema schema2({ ColumnSchema("col3", UINT32),
- ColumnSchema("col2", STRING) },
- 1);
- schema1.swap(schema2);
- ASSERT_EQ(2, schema1.num_columns());
- ASSERT_EQ(1, schema1.num_key_columns());
- ASSERT_EQ(3, schema2.num_columns());
- ASSERT_EQ(2, schema2.num_key_columns());
-}
-
TEST_F(TestSchema, TestColumnSchemaEquals) {
Slice default_str("read-write default");
ColumnSchema col1("key", STRING);
@@ -233,11 +278,10 @@ TEST_F(TestSchema, TestReset) {
1));
ASSERT_TRUE(schema.initialized());
- // Swap the initialized schema with an uninitialized one.
+ // Move an uninitialized schema into the initialized schema.
Schema schema2;
- schema2.swap(schema);
+ schema = std::move(schema2);
ASSERT_FALSE(schema.initialized());
- ASSERT_TRUE(schema2.initialized());
}
// Test for KUDU-943, a bug where we suspected that Variant didn't behave
http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index c999e6a..b15652e 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -141,8 +141,7 @@ size_t ColumnSchema::memory_footprint_including_this() const {
Schema::Schema(const Schema& other)
: name_to_index_bytes_(0),
- // TODO(adar): C++11 provides a single-arg constructor
- name_to_index_(10,
+ name_to_index_(/*bucket_count=*/10,
NameToIndexMap::hasher(),
NameToIndexMap::key_equal(),
NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -175,15 +174,45 @@ void Schema::CopyFrom(const Schema& other) {
has_nullables_ = other.has_nullables_;
}
-void Schema::swap(Schema& other) {
- std::swap(num_key_columns_, other.num_key_columns_);
- cols_.swap(other.cols_);
- col_ids_.swap(other.col_ids_);
- col_offsets_.swap(other.col_offsets_);
- name_to_index_.swap(other.name_to_index_);
- id_to_index_.swap(other.id_to_index_);
- std::swap(has_nullables_, other.has_nullables_);
+Schema::Schema(Schema&& other) noexcept
+ : cols_(std::move(other.cols_)),
+ num_key_columns_(other.num_key_columns_),
+ col_ids_(std::move(other.col_ids_)),
+ col_offsets_(std::move(other.col_offsets_)),
+ name_to_index_bytes_(0),
+ name_to_index_(/*bucket_count=*/10,
+ NameToIndexMap::hasher(),
+ NameToIndexMap::key_equal(),
+ NameToIndexMapAllocator(&name_to_index_bytes_)),
+ id_to_index_(std::move(other.id_to_index_)),
+ has_nullables_(other.has_nullables_) {
+ // 'name_to_index_' uses a customer allocator which holds a pointer to
+ // 'name_to_index_bytes_'. swap() will swap the contents but not the
+ // allocators; std::move will move the allocator[1], which will mean the moved
+ // to value will be holding a pointer to the moved-from's member, which is
+ // wrong and will likely lead to use-after-free. The 'name_to_index_bytes_'
+ // values should also be swapped so both schemas end up in a valid state.
+ //
+ // [1] STLCountingAllocator::propagate_on_container_move_assignment::value
+ // is std::true_type, which it inherits from the default Allocator.
std::swap(name_to_index_bytes_, other.name_to_index_bytes_);
+ name_to_index_.swap(other.name_to_index_);
+}
+
+Schema& Schema::operator=(Schema&& other) noexcept {
+ if (&other != this) {
+ cols_ = std::move(other.cols_);
+ num_key_columns_ = other.num_key_columns_;
+ col_ids_ = std::move(other.col_ids_);
+ col_offsets_ = std::move(other.col_offsets_);
+ id_to_index_ = std::move(other.id_to_index_);
+ has_nullables_ = other.has_nullables_;
+
+ // See the comment in the move constructor implementation for why we swap.
+ std::swap(name_to_index_bytes_, other.name_to_index_bytes_);
+ name_to_index_.swap(other.name_to_index_);
+ }
+ return *this;
}
Status Schema::Reset(const vector<ColumnSchema>& cols,
http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index b5e2003..449f3cd 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -400,10 +400,10 @@ class ColumnSchema {
// which prefix of columns makes up the primary key.
//
// Note that, while Schema is copyable and assignable, it is a complex
-// object that is not inexpensive to copy. You should generally prefer
-// passing by pointer or reference, and functions that create new
+// object that is not inexpensive to copy. Prefer move semantics when
+// possible, or pass by pointer or reference. Functions that create new
// Schemas should generally prefer taking a Schema pointer and using
-// Schema::swap() or Schema::Reset() rather than returning by value.
+// Schema::Reset() rather than returning by value.
class Schema {
public:
@@ -412,8 +412,10 @@ class Schema {
Schema()
: num_key_columns_(0),
name_to_index_bytes_(0),
- // TODO: C++11 provides a single-arg constructor
- name_to_index_(10,
+ // TODO(wdberkeley): C++11 provides a single-argument constructor, but
+ // it's not supported in GCC < 4.9. This (and the other occurrences here
+ // and in schema.cc) can be fixed if we adopt C++14 or a later standard.
+ name_to_index_(/*bucket_count=*/10,
NameToIndexMap::hasher(),
NameToIndexMap::key_equal(),
NameToIndexMapAllocator(&name_to_index_bytes_)),
@@ -423,9 +425,8 @@ class Schema {
Schema(const Schema& other);
Schema& operator=(const Schema& other);
- // TODO(todd) implement a move constructor
-
- void swap(Schema& other); // NOLINT(build/include_what_you_use)
+ Schema(Schema&& other) noexcept;
+ Schema& operator=(Schema&& other) noexcept;
void CopyFrom(const Schema& other);
@@ -438,8 +439,7 @@ class Schema {
Schema(const std::vector<ColumnSchema>& cols,
int key_columns)
: name_to_index_bytes_(0),
- // TODO: C++11 provides a single-arg constructor
- name_to_index_(10,
+ name_to_index_(/*bucket_count=*/10,
NameToIndexMap::hasher(),
NameToIndexMap::key_equal(),
NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -456,8 +456,7 @@ class Schema {
const std::vector<ColumnId>& ids,
int key_columns)
: name_to_index_bytes_(0),
- // TODO: C++11 provides a single-arg constructor
- name_to_index_(10,
+ name_to_index_(/*bucket_count=*/10,
NameToIndexMap::hasher(),
NameToIndexMap::key_equal(),
NameToIndexMapAllocator(&name_to_index_bytes_)) {
@@ -857,7 +856,6 @@ class Schema {
size_t memory_footprint_including_this() const;
private:
-
// Return a stringified version of the first 'num_columns' columns of the
// row.
template<class RowType>
@@ -906,8 +904,9 @@ class Schema {
// Cached indicator whether any columns are nullable.
bool has_nullables_;
- // NOTE: if you add more members, make sure to add the appropriate
- // code to swap() and CopyFrom() as well to prevent subtle bugs.
+ // NOTE: if you add more members, make sure to add the appropriate code to
+ // CopyFrom() and the move constructor and assignment operator as well, to
+ // prevent subtle bugs.
};
// Helper used for schema creation/editing.