tp: improve API of RuntimeTable
Phrasing as a Builder is a much more accurate way to do implement this
and prevents using Table APIs accidentally when the table is an
inconsistent state.
Change-Id: I60414c65d3ae62ca58f0bdfab3373ff304719c3c
diff --git a/src/base/test/status_matchers.h b/src/base/test/status_matchers.h
index e16214b..a6cddc2 100644
--- a/src/base/test/status_matchers.h
+++ b/src/base/test/status_matchers.h
@@ -36,13 +36,22 @@
return !arg.ok();
}
-// Macros for testing the results of functions that return absl::Status or
-// absl::StatusOr<T> (for any type T).
+// Macros for testing the results of functions that return base::Status or
+// base::StatusOr<T> (for any type T).
#define EXPECT_OK(expression) \
EXPECT_THAT(expression, ::perfetto::base::gtest_matchers::IsOk())
#define ASSERT_OK(expression) \
ASSERT_THAT(expression, ::perfetto::base::gtest_matchers::IsOk())
+// Macros for testing the results of function returning base::StatusOr<T>.
+#define PERFETTO_TEST_STATUS_MATCHER_CONCAT(x, y) x##y
+#define ASSERT_OK_AND_ASSIGN(lhs, rhs) \
+ PERFETTO_TEST_STATUS_MATCHER_CONCAT(auto status_or, __LINE__) = rhs; \
+ ASSERT_OK( \
+ PERFETTO_TEST_STATUS_MATCHER_CONCAT(status_or, __LINE__).status()); \
+ lhs = std::move( \
+ PERFETTO_TEST_STATUS_MATCHER_CONCAT(status_or, __LINE__).value())
+
} // namespace gtest_matchers
// Add a |PrintTo| function to allow easily determining what the cause of the
diff --git a/src/trace_processor/db/BUILD.gn b/src/trace_processor/db/BUILD.gn
index aca3a21..d870996 100644
--- a/src/trace_processor/db/BUILD.gn
+++ b/src/trace_processor/db/BUILD.gn
@@ -68,6 +68,8 @@
"../../../gn:gtest_and_gmock",
"../../../include/perfetto/trace_processor:basic_types",
"../../base",
+ "../../base:test_support",
+ "../containers",
"../tables",
"column",
"column:fake_storage",
diff --git a/src/trace_processor/db/runtime_table.cc b/src/trace_processor/db/runtime_table.cc
index a37d22a..6ccc942 100644
--- a/src/trace_processor/db/runtime_table.cc
+++ b/src/trace_processor/db/runtime_table.cc
@@ -28,11 +28,11 @@
#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
+#include "perfetto/ext/base/status_or.h"
#include "src/trace_processor/containers/string_pool.h"
#include "src/trace_processor/db/column.h"
-namespace perfetto {
-namespace trace_processor {
+namespace perfetto::trace_processor {
namespace {
template <typename T, typename U>
@@ -56,17 +56,17 @@
} // namespace
-RuntimeTable::RuntimeTable(StringPool* pool, std::vector<std::string> col_names)
- : Table(pool),
- col_names_(std::move(col_names)),
- storage_(col_names_.size()) {
- for (uint32_t i = 0; i < col_names_.size(); i++)
- storage_[i] = std::make_unique<VariantStorage>();
-}
-
RuntimeTable::~RuntimeTable() = default;
-base::Status RuntimeTable::AddNull(uint32_t idx) {
+RuntimeTable::Builder::Builder(StringPool* pool,
+ std::vector<std::string> col_names)
+ : string_pool_(pool), col_names_(std::move(col_names)) {
+ for (uint32_t i = 0; i < col_names_.size(); i++) {
+ storage_.emplace_back(std::make_unique<VariantStorage>());
+ }
+}
+
+base::Status RuntimeTable::Builder::AddNull(uint32_t idx) {
auto* col = storage_[idx].get();
PERFETTO_DCHECK(IsStorageNotIntNorDouble(*col));
if (auto* leading_nulls = std::get_if<uint32_t>(col)) {
@@ -83,7 +83,7 @@
return base::OkStatus();
}
-base::Status RuntimeTable::AddInteger(uint32_t idx, int64_t res) {
+base::Status RuntimeTable::Builder::AddInteger(uint32_t idx, int64_t res) {
auto* col = storage_[idx].get();
PERFETTO_DCHECK(IsStorageNotIntNorDouble(*col));
if (auto* leading_nulls_ptr = std::get_if<uint32_t>(col)) {
@@ -107,7 +107,7 @@
return base::OkStatus();
}
-base::Status RuntimeTable::AddFloat(uint32_t idx, double res) {
+base::Status RuntimeTable::Builder::AddFloat(uint32_t idx, double res) {
auto* col = storage_[idx].get();
PERFETTO_DCHECK(IsStorageNotIntNorDouble(*col));
if (auto* leading_nulls_ptr = std::get_if<uint32_t>(col)) {
@@ -139,7 +139,7 @@
return base::OkStatus();
}
-base::Status RuntimeTable::AddText(uint32_t idx, const char* ptr) {
+base::Status RuntimeTable::Builder::AddText(uint32_t idx, const char* ptr) {
auto* col = storage_[idx].get();
PERFETTO_DCHECK(IsStorageNotIntNorDouble(*col));
if (auto* leading_nulls_ptr = std::get_if<uint32_t>(col)) {
@@ -154,10 +154,16 @@
return base::OkStatus();
}
-base::Status RuntimeTable::AddColumnsAndOverlays(uint32_t rows) {
- overlays_.emplace_back(rows);
- for (uint32_t i = 0; i < col_names_.size(); ++i) {
- auto* col = storage_[i].get();
+base::StatusOr<std::unique_ptr<RuntimeTable>> RuntimeTable::Builder::Build(
+ uint32_t rows) && {
+ auto table = std::make_unique<RuntimeTable>();
+ table->row_count_ = rows;
+ table->string_pool_ = string_pool_;
+ table->overlays_.emplace_back(rows);
+ table->storage_ = std::move(storage_);
+ table->col_names_ = std::move(col_names_);
+ for (uint32_t i = 0; i < table->col_names_.size(); ++i) {
+ auto* col = table->storage_[i].get();
PERFETTO_DCHECK(IsStorageNotIntNorDouble(*col));
if (auto* leading_nulls = std::get_if<uint32_t>(col)) {
PERFETTO_CHECK(*leading_nulls == rows);
@@ -174,19 +180,18 @@
uint32_t flags = is_sorted ? ColumnLegacy::Flag::kNonNull |
ColumnLegacy::Flag::kSorted
: ColumnLegacy::Flag::kNonNull;
- columns_.push_back(ColumnLegacy(col_names_[i].c_str(), non_null_ints,
- flags, this, i, 0));
+ table->columns_.emplace_back(table->col_names_[i].c_str(),
+ non_null_ints, flags, table.get(), i, 0);
} else {
- columns_.push_back(ColumnLegacy(col_names_[i].c_str(), ints,
- ColumnLegacy::Flag::kNoFlag, this, i,
- 0));
+ table->columns_.emplace_back(table->col_names_[i].c_str(), ints,
+ ColumnLegacy::Flag::kNoFlag, table.get(),
+ i, 0);
}
-
} else if (auto* strings = std::get_if<StringStorage>(col)) {
PERFETTO_CHECK(strings->size() == rows);
- columns_.push_back(ColumnLegacy(col_names_[i].c_str(), strings,
- ColumnLegacy::Flag::kNonNull, this, i,
- 0));
+ table->columns_.emplace_back(table->col_names_[i].c_str(), strings,
+ ColumnLegacy::Flag::kNonNull, table.get(), i,
+ 0);
} else if (auto* doubles = std::get_if<NullDoubleStorage>(col)) {
PERFETTO_CHECK(doubles->size() == rows);
// Check if the column is nullable.
@@ -199,31 +204,29 @@
uint32_t flags = is_sorted ? ColumnLegacy::Flag::kNonNull |
ColumnLegacy::Flag::kSorted
: ColumnLegacy::Flag::kNonNull;
-
- columns_.push_back(ColumnLegacy(col_names_[i].c_str(), non_null_doubles,
- flags, this, i, 0));
+ table->columns_.emplace_back(table->col_names_[i].c_str(),
+ non_null_doubles, flags, table.get(), i,
+ 0);
} else {
- columns_.push_back(ColumnLegacy(col_names_[i].c_str(), doubles,
- ColumnLegacy::Flag::kNoFlag, this, i,
- 0));
+ table->columns_.emplace_back(table->col_names_[i].c_str(), doubles,
+ ColumnLegacy::Flag::kNoFlag, table.get(),
+ i, 0);
}
} else {
PERFETTO_FATAL("Unexpected column type");
}
}
- columns_.push_back(ColumnLegacy::IdColumn(
- this, static_cast<uint32_t>(col_names_.size()), 0, "_auto_id",
+ table->columns_.push_back(ColumnLegacy::IdColumn(
+ table.get(), static_cast<uint32_t>(table->columns_.size()), 0, "_auto_id",
ColumnLegacy::kIdFlags | ColumnLegacy::Flag::kHidden));
- row_count_ = rows;
- schema_.columns.reserve(columns_.size());
- for (const auto& col : columns_) {
- schema_.columns.emplace_back(Schema::Column{col.name(), col.type(),
- col.IsId(), col.IsSorted(),
- col.IsHidden(), col.IsSetId()});
+ table->schema_.columns.reserve(table->columns_.size());
+ for (const auto& col : table->columns_) {
+ table->schema_.columns.emplace_back(
+ Schema::Column{col.name(), col.type(), col.IsId(), col.IsSorted(),
+ col.IsHidden(), col.IsSetId()});
}
- return base::OkStatus();
+ return {std::move(table)};
}
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
diff --git a/src/trace_processor/db/runtime_table.h b/src/trace_processor/db/runtime_table.h
index 8678e68..f61b0dd 100644
--- a/src/trace_processor/db/runtime_table.h
+++ b/src/trace_processor/db/runtime_table.h
@@ -25,6 +25,7 @@
#include <vector>
#include "perfetto/base/status.h"
+#include "perfetto/ext/base/status_or.h"
#include "src/trace_processor/containers/string_pool.h"
#include "src/trace_processor/db/column_storage.h"
#include "src/trace_processor/db/table.h"
@@ -46,20 +47,25 @@
StringStorage,
DoubleStorage,
NullDoubleStorage>;
+ class Builder {
+ public:
+ Builder(StringPool* pool, std::vector<std::string> col_names);
- RuntimeTable(StringPool* pool, std::vector<std::string> col_names);
+ base::Status AddNull(uint32_t idx);
+ base::Status AddInteger(uint32_t idx, int64_t res);
+ base::Status AddFloat(uint32_t idx, double res);
+ base::Status AddText(uint32_t idx, const char* ptr);
+
+ base::StatusOr<std::unique_ptr<RuntimeTable>> Build(uint32_t rows) &&;
+
+ private:
+ StringPool* string_pool_ = nullptr;
+ std::vector<std::string> col_names_;
+ std::vector<std::unique_ptr<VariantStorage>> storage_;
+ };
+
~RuntimeTable() override;
- base::Status AddNull(uint32_t idx);
-
- base::Status AddInteger(uint32_t idx, int64_t res);
-
- base::Status AddFloat(uint32_t idx, double res);
-
- base::Status AddText(uint32_t idx, const char* ptr);
-
- base::Status AddColumnsAndOverlays(uint32_t rows);
-
const Table::Schema& schema() const { return schema_; }
private:
diff --git a/src/trace_processor/db/runtime_table_unittest.cc b/src/trace_processor/db/runtime_table_unittest.cc
index 85ca8ec..d5629ca 100644
--- a/src/trace_processor/db/runtime_table_unittest.cc
+++ b/src/trace_processor/db/runtime_table_unittest.cc
@@ -16,45 +16,51 @@
#include "src/trace_processor/db/runtime_table.h"
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "src/base/test/status_matchers.h"
+#include "src/trace_processor/containers/string_pool.h"
#include "test/gtest_and_gmock.h"
-namespace perfetto {
-namespace trace_processor {
+namespace perfetto::trace_processor {
namespace {
+using base::gtest_matchers::IsOk;
+using testing::Not;
class RuntimeTableTest : public ::testing::Test {
protected:
StringPool pool_;
std::vector<std::string> names_{{"foo"}};
- RuntimeTable table_{&pool_, names_};
+ RuntimeTable::Builder builder_{&pool_, names_};
};
TEST_F(RuntimeTableTest, DoubleThenIntValid) {
- ASSERT_TRUE(table_.AddFloat(0, 1024.3).ok());
- ASSERT_TRUE(table_.AddInteger(0, 1ll << 53).ok());
- ASSERT_TRUE(table_.AddColumnsAndOverlays(2).ok());
+ ASSERT_OK(builder_.AddFloat(0, 1024.3));
+ ASSERT_OK(builder_.AddInteger(0, 1ll << 53));
+ ASSERT_OK_AND_ASSIGN(auto table, std::move(builder_).Build(2));
- const auto& col = table_.columns()[0];
+ const auto& col = table->columns()[0];
ASSERT_EQ(col.Get(0).AsDouble(), 1024.3);
ASSERT_EQ(col.Get(1).AsDouble(), static_cast<double>(1ll << 53));
}
TEST_F(RuntimeTableTest, DoubleThenIntInvalid) {
- ASSERT_TRUE(table_.AddFloat(0, 1024.0).ok());
- ASSERT_FALSE(table_.AddInteger(0, (1ll << 53) + 1).ok());
- ASSERT_FALSE(table_.AddInteger(0, -(1ll << 53) - 1).ok());
+ ASSERT_OK(builder_.AddFloat(0, 1024.0));
+ ASSERT_THAT(builder_.AddInteger(0, (1ll << 53) + 1), Not(IsOk()));
+ ASSERT_THAT(builder_.AddInteger(0, -(1ll << 53) - 1), Not(IsOk()));
}
TEST_F(RuntimeTableTest, IntThenDouble) {
- ASSERT_TRUE(table_.AddInteger(0, 1024).ok());
- ASSERT_TRUE(table_.AddFloat(0, 1.3).ok());
- ASSERT_TRUE(table_.AddColumnsAndOverlays(2).ok());
+ ASSERT_TRUE(builder_.AddInteger(0, 1024).ok());
+ ASSERT_TRUE(builder_.AddFloat(0, 1.3).ok());
+ ASSERT_OK_AND_ASSIGN(auto table, std::move(builder_).Build(2));
- const auto& col = table_.columns()[0];
+ const auto& col = table->columns()[0];
ASSERT_EQ(col.Get(0).AsDouble(), 1024.0);
ASSERT_EQ(col.Get(1).AsDouble(), 1.3);
}
} // namespace
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
diff --git a/src/trace_processor/perfetto_sql/engine/perfetto_sql_engine.cc b/src/trace_processor/perfetto_sql/engine/perfetto_sql_engine.cc
index 086b42c..0ccc219 100644
--- a/src/trace_processor/perfetto_sql/engine/perfetto_sql_engine.cc
+++ b/src/trace_processor/perfetto_sql/engine/perfetto_sql_engine.cc
@@ -17,6 +17,8 @@
#include "src/trace_processor/perfetto_sql/engine/perfetto_sql_engine.h"
#include <cctype>
+#include <cstddef>
+#include <cstdint>
#include <memory>
#include <optional>
#include <string>
@@ -29,6 +31,7 @@
#include "perfetto/ext/base/status_or.h"
#include "perfetto/ext/base/string_utils.h"
#include "perfetto/ext/base/string_view.h"
+#include "src/trace_processor/db/runtime_table.h"
#include "src/trace_processor/db/table.h"
#include "src/trace_processor/perfetto_sql/engine/created_function.h"
#include "src/trace_processor/perfetto_sql/engine/function_util.h"
@@ -409,7 +412,7 @@
"CREATE PERFETTO TABLE"));
size_t column_count = column_names.size();
- auto table = std::make_unique<RuntimeTable>(pool_, std::move(column_names));
+ RuntimeTable::Builder builder(pool_, std::move(column_names));
uint32_t rows = 0;
int res;
for (res = sqlite3_step(stmt.sqlite_stmt()); res == SQLITE_ROW;
@@ -418,18 +421,18 @@
int int_i = static_cast<int>(i);
switch (sqlite3_column_type(stmt.sqlite_stmt(), int_i)) {
case SQLITE_NULL:
- RETURN_IF_ERROR(table->AddNull(i));
+ RETURN_IF_ERROR(builder.AddNull(i));
break;
case SQLITE_INTEGER:
- RETURN_IF_ERROR(table->AddInteger(
+ RETURN_IF_ERROR(builder.AddInteger(
i, sqlite3_column_int64(stmt.sqlite_stmt(), int_i)));
break;
case SQLITE_FLOAT:
- RETURN_IF_ERROR(table->AddFloat(
+ RETURN_IF_ERROR(builder.AddFloat(
i, sqlite3_column_double(stmt.sqlite_stmt(), int_i)));
break;
case SQLITE_TEXT: {
- RETURN_IF_ERROR(table->AddText(
+ RETURN_IF_ERROR(builder.AddText(
i, reinterpret_cast<const char*>(
sqlite3_column_text(stmt.sqlite_stmt(), int_i))));
break;
@@ -448,7 +451,7 @@
create_table.name.c_str(),
sqlite3_errmsg(engine_->db()));
}
- RETURN_IF_ERROR(table->AddColumnsAndOverlays(rows));
+ ASSIGN_OR_RETURN(auto table, std::move(builder).Build(rows));
if (runtime_tables_.Find(create_table.name)) {
if (!create_table.replace) {
diff --git a/src/trace_processor/util/status_macros.h b/src/trace_processor/util/status_macros.h
index d7acb98..c902bac 100644
--- a/src/trace_processor/util/status_macros.h
+++ b/src/trace_processor/util/status_macros.h
@@ -38,6 +38,6 @@
PERFETTO_INTERNAL_MACRO_CONCAT(auto status_or, __LINE__) = rhs; \
RETURN_IF_ERROR( \
PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__).status()); \
- lhs = std::move(rhs.value())
+ lhs = std::move(PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__).value())
#endif // SRC_TRACE_PROCESSOR_UTIL_STATUS_MACROS_H_
