This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new add7cb9 [common] style update on schema.{cc,h} and partition.cc
add7cb9 is described below
commit add7cb99fc8d6ab586e27d0ecd7d1698f0f45ae3
Author: Alexey Serbin <[email protected]>
AuthorDate: Fri Jul 30 13:03:45 2021 -0700
[common] style update on schema.{cc,h} and partition.cc
While I was updating the code around, I decided to separate changes
related to the project's style guide and comparison of signed/unsigned
integers into a separate patch. In addition, I updated the code to
consistently use dedicated constants (like kColumnNotFound) throughout
the code in schema.cc.
This changelist doesn't contain any functional changes.
Change-Id: I55d02fed53a39d311b43ea7bd956316abc998f60
Reviewed-on: http://gerrit.cloudera.org:8080/17742
Reviewed-by: Andrew Wong <[email protected]>
Tested-by: Kudu Jenkins
---
src/kudu/common/partition.cc | 54 +++++++++++++++++-----------------
src/kudu/common/schema.cc | 45 ++++++++++++++---------------
src/kudu/common/schema.h | 69 ++++++++++++++++++++------------------------
3 files changed, 82 insertions(+), 86 deletions(-)
diff --git a/src/kudu/common/partition.cc b/src/kudu/common/partition.cc
index 57ba3ef..f2b5c55 100644
--- a/src/kudu/common/partition.cc
+++ b/src/kudu/common/partition.cc
@@ -246,7 +246,7 @@ Status PartitionSchema::FromPB(const PartitionSchemaPB& pb,
// Fill in the default range partition (PK columns).
// like the sorting above, this should only happen during table creation
// while deserializing the user-provided partition schema.
- for (int32_t column_idx = 0; column_idx < schema.num_key_columns();
column_idx++) {
+ for (size_t column_idx = 0; column_idx < schema.num_key_columns();
++column_idx) {
partition_schema->range_schema_.column_ids.push_back(schema.column_id(column_idx));
}
}
@@ -334,7 +334,7 @@ Status PartitionSchema::EncodeRangeKey(const
KuduPartialRow& row,
string* key) const {
DCHECK(key->empty());
bool contains_no_columns = true;
- for (int column_idx = 0; column_idx < schema.num_columns(); column_idx++) {
+ for (size_t column_idx = 0; column_idx < schema.num_columns(); ++column_idx)
{
const ColumnSchema& column = schema.column(column_idx);
if (row.IsColumnSet(column_idx)) {
if (std::find(range_schema_.column_ids.begin(),
@@ -384,9 +384,11 @@ Status PartitionSchema::EncodeRangeBounds(
const PerRangeHashBucketSchemas& range_hash_schemas,
const Schema& schema,
RangesWithHashSchemas* bounds_with_hash_schemas) const {
- DCHECK(bounds_with_hash_schemas->empty());
+ DCHECK(bounds_with_hash_schemas);
+ auto& bounds_whs = *bounds_with_hash_schemas;
+ DCHECK(bounds_whs.empty());
if (range_bounds.empty()) {
- bounds_with_hash_schemas->emplace_back(RangeWithHashSchemas{"", "", {}});
+ bounds_whs.emplace_back(RangeWithHashSchemas{"", "", {}});
return Status::OK();
}
@@ -414,21 +416,18 @@ Status PartitionSchema::EncodeRangeBounds(
if (!range_hash_schemas.empty()) {
temp.hash_schemas = range_hash_schemas[j++];
}
- bounds_with_hash_schemas->emplace_back(std::move(temp));
+ bounds_whs.emplace_back(std::move(temp));
}
- std::sort(bounds_with_hash_schemas->begin(), bounds_with_hash_schemas->end(),
+ std::sort(bounds_whs.begin(), bounds_whs.end(),
[](const RangeWithHashSchemas& s1, const RangeWithHashSchemas& s2)
{
return s1.lower < s2.lower;
});
- // Check that the range bounds are non-overlapping
- if (bounds_with_hash_schemas->empty()) {
- return Status::OK();
- }
- for (int i = 0; i < bounds_with_hash_schemas->size() - 1; i++) {
- const string& first_upper = bounds_with_hash_schemas->at(i).upper;
- const string& second_lower = bounds_with_hash_schemas->at(i + 1).lower;
+ // Check that the range bounds are non-overlapping.
+ for (size_t i = 0; i + 1 < bounds_whs.size(); ++i) {
+ const string& first_upper = bounds_whs[i].upper;
+ const string& second_lower = bounds_whs[i + 1].lower;
if (first_upper.empty() || second_lower.empty() ||
first_upper > second_lower) {
@@ -436,11 +435,11 @@ Status PartitionSchema::EncodeRangeBounds(
"overlapping range partitions",
Substitute(
"first range partition: $0, second range partition: $1",
- RangePartitionDebugString(bounds_with_hash_schemas->at(i).lower,
- bounds_with_hash_schemas->at(i).upper,
+ RangePartitionDebugString(bounds_whs[i].lower,
+ bounds_whs[i].upper,
schema),
- RangePartitionDebugString(bounds_with_hash_schemas->at(i +
1).lower,
- bounds_with_hash_schemas->at(i +
1).upper,
+ RangePartitionDebugString(bounds_whs[i + 1].lower,
+ bounds_whs[i + 1].upper,
schema)));
}
}
@@ -468,7 +467,7 @@ Status PartitionSchema::SplitRangeBounds(
string& lower = bound.lower;
const string& upper = bound.upper;
- for (; split != splits.end() && (upper.empty() || *split <= upper);
split++) {
+ for (; split != splits.end() && (upper.empty() || *split <= upper);
++split) {
if (!lower.empty() && *split < lower) {
return Status::InvalidArgument("split out of bounds",
RangeKeyDebugString(*split, schema));
}
@@ -570,7 +569,7 @@ Status PartitionSchema::CreatePartitions(
DCHECK(split_rows.empty());
vector<Partition> result_partitions;
// Iterate through each bound and its hash schemas to generate hash
partitions.
- for (int i = 0; i < bounds_with_hash_schemas.size(); i++) {
+ for (size_t i = 0; i < bounds_with_hash_schemas.size(); ++i) {
const auto& bound = bounds_with_hash_schemas[i];
const auto& current_range_hash_schemas = bound.hash_schemas;
// If current bound's HashBucketSchema is empty, implies use of default
@@ -1147,11 +1146,13 @@ bool PartitionSchema::operator==(const PartitionSchema&
rhs) const {
Status PartitionSchema::EncodeColumns(const ConstContiguousRow& row,
const vector<ColumnId>& column_ids,
string* buf) {
- for (int i = 0; i < column_ids.size(); i++) {
+ for (size_t i = 0; i < column_ids.size(); ++i) {
ColumnId column_id = column_ids[i];
- int32_t column_idx = row.schema()->find_column_by_id(column_id);
+ auto column_idx = row.schema()->find_column_by_id(column_id);
+ CHECK(column_idx != Schema::kColumnNotFound);
const TypeInfo* type = row.schema()->column(column_idx).type_info();
- GetKeyEncoder<string>(type).Encode(row.cell_ptr(column_idx), i + 1 ==
column_ids.size(), buf);
+ GetKeyEncoder<string>(type).Encode(
+ row.cell_ptr(column_idx), i + 1 == column_ids.size(), buf);
}
return Status::OK();
}
@@ -1160,8 +1161,8 @@ Status PartitionSchema::EncodeColumns(const
ConstContiguousRow& row,
Status PartitionSchema::EncodeColumns(const KuduPartialRow& row,
const vector<ColumnId>& column_ids,
string* buf) {
- for (int i = 0; i < column_ids.size(); i++) {
- int32_t column_idx = row.schema()->find_column_by_id(column_ids[i]);
+ for (size_t i = 0; i < column_ids.size(); ++i) {
+ auto column_idx = row.schema()->find_column_by_id(column_ids[i]);
CHECK(column_idx != Schema::kColumnNotFound);
const TypeInfo* type_info = row.schema()->column(column_idx).type_info();
const KeyEncoder<string>& encoder = GetKeyEncoder<string>(type_info);
@@ -1172,7 +1173,8 @@ Status PartitionSchema::EncodeColumns(const
KuduPartialRow& row,
encoder.Encode(min_value, i + 1 == column_ids.size(), buf);
} else {
ContiguousRow cont_row(row.schema(), row.row_data_);
- encoder.Encode(cont_row.cell_ptr(column_idx), i + 1 ==
column_ids.size(), buf);
+ encoder.Encode(
+ cont_row.cell_ptr(column_idx), i + 1 == column_ids.size(), buf);
}
}
return Status::OK();
@@ -1578,7 +1580,7 @@ Status PartitionSchema::GetRangeSchemaColumnIndexes(const
Schema& schema,
int32_t PartitionSchema::TryGetSingleColumnHashPartitionIndex(const Schema&
schema,
int32_t col_idx)
const {
const ColumnId column_id = schema.column_id(col_idx);
- for (int i = 0; i < hash_partition_schemas().size(); ++i) {
+ for (size_t i = 0; i < hash_partition_schemas().size(); ++i) {
auto hash_partition = hash_partition_schemas()[i];
if (hash_partition.column_ids.size() == 1 && hash_partition.column_ids[0]
== column_id) {
return i;
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index 38c0ce5..a8df9f6 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -132,8 +132,8 @@ Status ColumnSchema::ApplyDelta(const ColumnSchemaDelta&
col_delta) {
if (col_delta.default_value) {
const void* value = type_info()->physical_type() == BINARY ?
- reinterpret_cast<const void
*>(&col_delta.default_value.get()) :
- reinterpret_cast<const void
*>(col_delta.default_value->data());
+ reinterpret_cast<const
void*>(&col_delta.default_value.get()) :
+ reinterpret_cast<const
void*>(col_delta.default_value->data());
write_default_ = std::make_shared<Variant>(type_info()->type(), value);
}
@@ -188,11 +188,9 @@ size_t ColumnSchema::memory_footprint_including_this()
const {
return kudu_malloc_usable_size(this) + memory_footprint_excluding_this();
}
-const int Schema::kColumnNotFound = -1;
-
Schema::Schema(const Schema& other)
- : name_to_index_(other.num_columns()) {
- name_to_index_.set_empty_key(StringPiece());
+ : name_to_index_(other.num_columns()) {
+ name_to_index_.set_empty_key(StringPiece());
CopyFrom(other);
}
@@ -204,6 +202,7 @@ Schema& Schema::operator=(const Schema& other) {
}
void Schema::CopyFrom(const Schema& other) {
+ DCHECK_NE(this, &other);
num_key_columns_ = other.num_key_columns_;
cols_ = other.cols_;
col_ids_ = other.col_ids_;
@@ -214,8 +213,8 @@ void Schema::CopyFrom(const Schema& other) {
// We can't simply copy name_to_index_ since the StringPiece keys
// reference the other Schema's ColumnSchema objects.
name_to_index_.clear_no_resize();
- int i = 0;
- for (const ColumnSchema &col : cols_) {
+ size_t i = 0;
+ for (const auto& col : cols_) {
// The map uses the 'name' string from within the ColumnSchema object.
name_to_index_[col.name()] = i++;
}
@@ -286,7 +285,7 @@ Status Schema::Reset(vector<ColumnSchema> cols,
size_t off = 0;
size_t i = 0;
name_to_index_.clear_no_resize();
- for (const ColumnSchema &col : cols_) {
+ for (const ColumnSchema& col : cols_) {
if (col.name().empty()) {
return Status::InvalidArgument("column names must be non-empty");
}
@@ -307,7 +306,7 @@ Status Schema::Reset(vector<ColumnSchema> cols,
col_ids_ = std::move(ids);
id_to_index_.clear();
max_col_id_ = 0;
- for (int i = 0; i < col_ids_.size(); ++i) {
+ for (size_t i = 0; i < col_ids_.size(); ++i) {
if (col_ids_[i] > max_col_id_) {
max_col_id_ = col_ids_[i];
}
@@ -339,13 +338,13 @@ Status Schema::FindColumn(Slice col_name, int* idx) const
{
return Status::OK();
}
-Status Schema::CreateProjectionByNames(const std::vector<StringPiece>&
col_names,
+Status Schema::CreateProjectionByNames(const vector<StringPiece>& col_names,
Schema* out) const {
vector<ColumnId> ids;
vector<ColumnSchema> cols;
for (const StringPiece& name : col_names) {
- int idx = find_column(name);
- if (idx == -1) {
+ const int idx = find_column(name);
+ if (idx == kColumnNotFound) {
return Status::NotFound("column not found", name);
}
if (has_column_ids()) {
@@ -356,13 +355,13 @@ Status Schema::CreateProjectionByNames(const
std::vector<StringPiece>& col_names
return out->Reset(std::move(cols), std::move(ids), 0);
}
-Status Schema::CreateProjectionByIdsIgnoreMissing(const std::vector<ColumnId>&
col_ids,
+Status Schema::CreateProjectionByIdsIgnoreMissing(const vector<ColumnId>&
col_ids,
Schema* out) const {
vector<ColumnSchema> cols;
vector<ColumnId> filtered_col_ids;
for (ColumnId id : col_ids) {
- int idx = find_column_by_id(id);
- if (idx == -1) {
+ const auto idx = find_column_by_id(id);
+ if (idx == kColumnNotFound) {
continue;
}
cols.push_back(column(idx));
@@ -418,7 +417,7 @@ Status Schema::VerifyProjectionCompatibility(const Schema&
projection) const {
Status Schema::GetMappedReadProjection(const Schema& projection,
- Schema *mapped_projection) const {
+ Schema* mapped_projection) const {
// - The user projection may have different columns from the ones on the
tablet
// - User columns non present in the tablet are considered errors
// - The user projection is not supposed to have the defaults or the nullable
@@ -460,7 +459,7 @@ string Schema::ToString(ToStringMode mode) const {
vector<string> pk_strs;
pk_strs.reserve(num_key_columns_);
- for (int i = 0; i < num_key_columns_; i++) {
+ for (size_t i = 0; i < num_key_columns_; ++i) {
pk_strs.push_back(cols_[i].name());
}
@@ -470,11 +469,11 @@ string Schema::ToString(ToStringMode mode) const {
}
vector<string> col_strs;
if (has_column_ids() && (mode & ToStringMode::WITH_COLUMN_IDS)) {
- for (int i = 0; i < cols_.size(); ++i) {
+ for (size_t i = 0; i < cols_.size(); ++i) {
col_strs.push_back(Substitute("$0:$1", col_ids_[i],
cols_[i].ToString(col_mode)));
}
} else {
- for (const ColumnSchema &col : cols_) {
+ for (const ColumnSchema& col : cols_) {
col_strs.push_back(col.ToString(col_mode));
}
}
@@ -591,8 +590,8 @@ Status SchemaBuilder::AddKeyColumn(const string& name,
DataType type) {
Status SchemaBuilder::AddColumn(const string& name,
DataType type,
bool is_nullable,
- const void *read_default,
- const void *write_default) {
+ const void* read_default,
+ const void* write_default) {
if (name.empty()) {
return Status::InvalidArgument("column name must be non-empty");
}
@@ -659,7 +658,7 @@ Status SchemaBuilder::AddColumn(const ColumnSchema& column,
bool is_key) {
if (is_key) {
cols_.insert(cols_.begin() + num_key_columns_, column);
col_ids_.insert(col_ids_.begin() + num_key_columns_, next_id_);
- num_key_columns_++;
+ ++num_key_columns_;
} else {
cols_.push_back(column);
col_ids_.push_back(next_id_);
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index d6e48dd..e44e018 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -14,8 +14,8 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#ifndef KUDU_COMMON_SCHEMA_H
-#define KUDU_COMMON_SCHEMA_H
+
+#pragma once
#include <cstddef>
#include <cstdint>
@@ -82,8 +82,8 @@ struct ColumnId {
operator const int32_t() const { return t_; } // NOLINT
operator const strings::internal::SubstituteArg() const { return t_; } //
NOLINT
operator const AlphaNum() const { return t_; } // NOLINT
- bool operator==(const ColumnId & rhs) const { return t_ == rhs.t_; }
- bool operator<(const ColumnId & rhs) const { return t_ < rhs.t_; }
+ bool operator==(const ColumnId& rhs) const { return t_ == rhs.t_; }
+ bool operator<(const ColumnId& rhs) const { return t_ < rhs.t_; }
friend std::ostream& operator<<(std::ostream& os, ColumnId column_id) {
return os << column_id.t_;
}
@@ -249,7 +249,7 @@ class ColumnSchema {
return is_nullable_;
}
- const std::string &name() const {
+ const std::string& name() const {
return name_;
}
@@ -287,9 +287,9 @@ class ColumnSchema {
// The returned value will be valid until the ColumnSchema will be destroyed.
//
// Example:
- // const uint32_t *vu32 = static_cast<const uint32_t
*>(col_schema.read_default_value());
- // const Slice *vstr = static_cast<const Slice
*>(col_schema.read_default_value());
- const void *read_default_value() const {
+ // const uint32_t* vu32 = static_cast<const
uint32_t*>(col_schema.read_default_value());
+ // const Slice* vstr = static_cast<const
Slice*>(col_schema.read_default_value());
+ const void* read_default_value() const {
if (read_default_ != nullptr) {
return read_default_->value();
}
@@ -306,9 +306,9 @@ class ColumnSchema {
// The returned value will be valid until the ColumnSchema will be destroyed.
//
// Example:
- // const uint32_t *vu32 = static_cast<const uint32_t
*>(col_schema.write_default_value());
- // const Slice *vstr = static_cast<const Slice
*>(col_schema.write_default_value());
- const void *write_default_value() const {
+ // const uint32_t* vu32 = static_cast<const
uint32_t*>(col_schema.write_default_value());
+ // const Slice* vstr = static_cast<const
Slice*>(col_schema.write_default_value());
+ const void* write_default_value() const {
if (write_default_ != nullptr) {
return write_default_->value();
}
@@ -321,7 +321,7 @@ class ColumnSchema {
type_info()->physical_type() == other.type_info()->physical_type();
}
- bool EqualsType(const ColumnSchema &other) const {
+ bool EqualsType(const ColumnSchema& other) const {
if (this == &other) return true;
return is_nullable_ == other.is_nullable_ &&
type_info()->type() == other.type_info()->type() &&
@@ -338,7 +338,7 @@ class ColumnSchema {
COMPARE_ALL = COMPARE_NAME | COMPARE_TYPE | COMPARE_OTHER
};
- bool Equals(const ColumnSchema &other,
+ bool Equals(const ColumnSchema& other,
CompareFlags flags = COMPARE_ALL) const {
if (this == &other) return true;
@@ -388,13 +388,13 @@ class ColumnSchema {
return type_attributes_;
}
- int Compare(const void *lhs, const void *rhs) const {
+ int Compare(const void* lhs, const void* rhs) const {
return type_info_->Compare(lhs, rhs);
}
// Stringify the given cell. This just stringifies the cell contents,
// and doesn't include the column name or type.
- std::string Stringify(const void *cell) const {
+ std::string Stringify(const void* cell) const {
std::string ret;
type_info_->AppendDebugStringForValue(cell, &ret);
return ret;
@@ -431,7 +431,7 @@ class ColumnSchema {
}
std::string name_;
- const TypeInfo *type_info_;
+ const TypeInfo* type_info_;
bool is_nullable_;
// use shared_ptr since the ColumnSchema is always copied around.
std::shared_ptr<Variant> read_default_;
@@ -453,8 +453,7 @@ class ColumnSchema {
// Schema::Reset() rather than returning by value.
class Schema {
public:
-
- static const int kColumnNotFound;
+ static constexpr int kColumnNotFound = -1;
Schema()
: num_key_columns_(0),
@@ -558,7 +557,7 @@ class Schema {
}
// Return the ColumnSchema corresponding to the given column index.
- const ColumnSchema &column(size_t idx) const {
+ const ColumnSchema& column(size_t idx) const {
DCHECK_LT(idx, cols_.size());
return cols_[idx];
}
@@ -640,23 +639,22 @@ class Schema {
// are off, incorrect data may result.
//
// This is mostly useful for tests at this point.
- // TODO: consider removing it.
template<DataType Type, class RowType>
- const typename DataTypeTraits<Type>::cpp_type *
+ const typename DataTypeTraits<Type>::cpp_type*
ExtractColumnFromRow(const RowType& row, size_t idx) const {
DCHECK_SCHEMA_EQ(*this, *row.schema());
const ColumnSchema& col_schema = cols_[idx];
DCHECK_LT(idx, cols_.size());
DCHECK_EQ(col_schema.type_info()->type(), Type);
- const void *val;
+ const void* val;
if (col_schema.is_nullable()) {
val = row.nullable_cell_ptr(idx);
} else {
val = row.cell_ptr(idx);
}
- return reinterpret_cast<const typename DataTypeTraits<Type>::cpp_type
*>(val);
+ return reinterpret_cast<const typename
DataTypeTraits<Type>::cpp_type*>(val);
}
// Stringify the given row, which conforms to this schema,
@@ -762,7 +760,7 @@ class Schema {
// contents.
// Returns the encoded key.
template <class RowType>
- Slice EncodeComparableKey(const RowType& row, faststring *dst) const {
+ Slice EncodeComparableKey(const RowType& row, faststring* dst) const {
DCHECK_KEY_PROJECTION_SCHEMA_EQ(*this, *row.schema());
dst->clear();
@@ -845,7 +843,7 @@ class Schema {
// Returns the projection schema mapped on the current one
// If the project is invalid, return a non-OK status.
Status GetMappedReadProjection(const Schema& projection,
- Schema *mapped_projection) const;
+ Schema* mapped_projection) const;
// Loops through this schema (the projection) and calls the projector
methods once for
// each column.
@@ -870,7 +868,7 @@ class Schema {
//
// TODO(MAYBE): Pass the ColumnSchema and not only the column index?
template <class Projector>
- Status GetProjectionMapping(const Schema& base_schema, Projector *projector)
const {
+ Status GetProjectionMapping(const Schema& base_schema, Projector* projector)
const {
const bool use_column_ids = base_schema.has_column_ids() &&
has_column_ids();
int proj_idx = 0;
@@ -918,7 +916,7 @@ class Schema {
int find_column_by_id(ColumnId id) const {
DCHECK(cols_.empty() || has_column_ids());
int ret = id_to_index_[id];
- if (ret == -1) {
+ if (ret == IdMapping::kNoEntry) {
return kColumnNotFound;
}
return ret;
@@ -994,11 +992,10 @@ class Schema {
// Helper used for schema creation/editing.
//
// Example:
-// Status s;
// SchemaBuilder builder(base_schema);
-// s = builder.RemoveColumn("value");
-// s = builder.AddKeyColumn("key2", STRING);
-// s = builder.AddColumn("new_c1", UINT32);
+// RETURN_NOT_OK(builder.RemoveColumn("value"));
+// RETURN_NOT_OK(builder.AddKeyColumn("key2", STRING));
+// RETURN_NOT_OK(builder.AddColumn("new_c1", UINT32));
// ...
// Schema new_schema = builder.Build();
class SchemaBuilder {
@@ -1009,7 +1006,7 @@ class SchemaBuilder {
void Reset();
void Reset(const Schema& schema);
- bool is_valid() const { return cols_.size() > 0; }
+ bool is_valid() const { return !cols_.empty(); }
// Set the next column ID to be assigned to columns added with
// AddColumn.
@@ -1027,7 +1024,7 @@ class SchemaBuilder {
// Return false if the column is not a key column, or if
// the column is not in the builder.
bool is_key_column(const StringPiece col_name) const {
- for (int i = 0; i < num_key_columns_; i++) {
+ for (size_t i = 0; i < num_key_columns_; ++i) {
if (cols_[i].name() == col_name) return true;
}
return false;
@@ -1051,8 +1048,8 @@ class SchemaBuilder {
Status AddColumn(const std::string& name,
DataType type,
bool is_nullable,
- const void *read_default,
- const void *write_default);
+ const void* read_default,
+ const void* write_default);
Status RemoveColumn(const std::string& name);
@@ -1081,5 +1078,3 @@ struct hash<kudu::ColumnId> {
}
};
} // namespace std
-
-#endif
