feat!: refactor Read to return ReadResult; remove ResultSet (#935)

Author: scotthart

google/cloud/spanner/client.cc

@@ -27,32 +27,33 @@ namespace cloud {
 namespace spanner {
 inline namespace SPANNER_CLIENT_NS {
 
-StatusOr<ResultSet> Client::Read(std::string table, KeySet keys,
-                                 std::vector<std::string> columns,
-                                 ReadOptions read_options) {
+ReadResult Client::Read(std::string table, KeySet keys,
+                        std::vector<std::string> columns,
+                        ReadOptions read_options) {
   return conn_->Read(
       {internal::MakeSingleUseTransaction(Transaction::ReadOnlyOptions()),
        std::move(table), std::move(keys), std::move(columns),
        std::move(read_options)});
 }
 
-StatusOr<ResultSet> Client::Read(
-    Transaction::SingleUseOptions transaction_options, std::string table,
-    KeySet keys, std::vector<std::string> columns, ReadOptions read_options) {
+ReadResult Client::Read(Transaction::SingleUseOptions transaction_options,
+                        std::string table, KeySet keys,
+                        std::vector<std::string> columns,
+                        ReadOptions read_options) {
   return conn_->Read(
       {internal::MakeSingleUseTransaction(std::move(transaction_options)),
        std::move(table), std::move(keys), std::move(columns),
        std::move(read_options)});
 }
 
-StatusOr<ResultSet> Client::Read(Transaction transaction, std::string table,
-                                 KeySet keys, std::vector<std::string> columns,
-                                 ReadOptions read_options) {
+ReadResult Client::Read(Transaction transaction, std::string table, KeySet keys,
+                        std::vector<std::string> columns,
+                        ReadOptions read_options) {
   return conn_->Read({std::move(transaction), std::move(table), std::move(keys),
                       std::move(columns), std::move(read_options)});
 }
 
-StatusOr<ResultSet> Client::Read(ReadPartition const& read_partition) {
+ReadResult Client::Read(ReadPartition const& read_partition) {
   return conn_->Read(internal::MakeReadParams(read_partition));
 }
 

google/cloud/spanner/client.h

@@ -75,16 +75,12 @@ inline namespace SPANNER_CLIENT_NS {
  *
  * @code
  * namespace cs = ::google::cloud::spanner;
- * using ::google::cloud::StatusOr;
  *
  * auto db = cs::Database("my_project", "my_instance", "my_db_id"));
  * auto conn = cs::MakeConnection(db);
  * auto client = cs::Client(conn);
  *
- * StatusOr<cs::ResultSet> result = client.Read(...);
- * if (!result) {
- *   return result.status();
- * }
+ * cs::ReadResult result = client.Read(...);
  * using RowType = Row<std::int64_t, std::string>;
  * for (auto const& row : result.Rows<RowType>()) {
  *   // ...
@@ -143,31 +139,30 @@ class Client {
    *     this request.
    * @param read_options `ReadOptions` used for this request.
    *
-   * @return A `StatusOr` containing a `ResultSet` or error status on failure.
-   *     No individual row in the `ResultSet` can exceed 100 MiB, and no column
-   *     value can exceed 10 MiB.
+   * @note No individual row in the `ReadResult` can exceed 100 MiB, and no
+   *     column value can exceed 10 MiB.
    */
-  StatusOr<ResultSet> Read(std::string table, KeySet keys,
-                           std::vector<std::string> columns,
-                           ReadOptions read_options = {});
+  ReadResult Read(std::string table, KeySet keys,
+                  std::vector<std::string> columns,
+                  ReadOptions read_options = {});
   /**
    * @copydoc Read
    *
    * @param transaction_options Execute this read in a single-use transaction
    * with these options.
    */
-  StatusOr<ResultSet> Read(Transaction::SingleUseOptions transaction_options,
-                           std::string table, KeySet keys,
-                           std::vector<std::string> columns,
-                           ReadOptions read_options = {});
+  ReadResult Read(Transaction::SingleUseOptions transaction_options,
+                  std::string table, KeySet keys,
+                  std::vector<std::string> columns,
+                  ReadOptions read_options = {});
   /**
    * @copydoc Read
    *
    * @param transaction Execute this read as part of an existing transaction.
    */
-  StatusOr<ResultSet> Read(Transaction transaction, std::string table,
-                           KeySet keys, std::vector<std::string> columns,
-                           ReadOptions read_options = {});
+  ReadResult Read(Transaction transaction, std::string table, KeySet keys,
+                  std::vector<std::string> columns,
+                  ReadOptions read_options = {});
   //@}
 
   /**
@@ -177,14 +172,13 @@ class Client {
    *
    * @param partition A `ReadPartition`, obtained by calling `PartitionRead`.
    *
-   * @return A `StatusOr` containing a `ResultSet` or error status on failure.
-   *     No individual row in the `ResultSet` can exceed 100 MiB, and no column
-   *     value can exceed 10 MiB.
+   * @note No individual row in the `ReadResult` can exceed 100 MiB, and no
+   *     column value can exceed 10 MiB.
    *
    * @par Example
    * @snippet samples.cc read-read-partition
    */
-  StatusOr<ResultSet> Read(ReadPartition const& partition);
+  ReadResult Read(ReadPartition const& partition);
 
   /**
    * Creates a set of partitions that can be used to execute a read
@@ -237,8 +231,8 @@ class Client {
    *
    * @param statement The SQL statement to execute.
    *
-   * @note No individual row in the `ResultSet` can exceed 100 MiB, and no
-   * column value can exceed 10 MiB.
+   * @note No individual row in the `ExecuteQueryResult` can exceed 100 MiB, and
+   * no column value can exceed 10 MiB.
    */
   ExecuteQueryResult ExecuteQuery(SqlStatement statement);
 
@@ -266,8 +260,8 @@ class Client {
    *
    * @param partition A `QueryPartition`, obtained by calling `PartitionRead`.
    *
-   * @note No individual row in the `ResultSet` can exceed 100 MiB, and no
-   * column value can exceed 10 MiB.
+   * @note No individual row in the `ExecuteQueryResult` can exceed 100 MiB, and
+   * no column value can exceed 10 MiB.
    *
    * @par Example
    * @snippet samples.cc execute-sql-query-partition

google/cloud/spanner/client_test.cc

@@ -96,29 +96,26 @@ TEST(ClientTest, ReadSuccess) {
       )pb",
       &metadata));
   EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
-  EXPECT_CALL(*source, Stats())
-      .WillRepeatedly(Return(optional<spanner_proto::ResultSetStats>()));
   EXPECT_CALL(*source, NextValue())
       .WillOnce(Return(optional<Value>("Steve")))
       .WillOnce(Return(optional<Value>(12)))
       .WillOnce(Return(optional<Value>("Ann")))
       .WillOnce(Return(optional<Value>(42)))
       .WillOnce(Return(optional<Value>()));
 
-  ResultSet result_set(std::move(source));
+  ReadResult result_set(std::move(source));
   EXPECT_CALL(*conn, Read(_)).WillOnce(Return(ByMove(std::move(result_set))));
 
   KeySet keys = KeySet::All();
   auto result = client.Read("table", std::move(keys), {"column1", "column2"});
-  EXPECT_STATUS_OK(result);
 
   using RowType = Row<std::string, std::int64_t>;
   auto expected = std::vector<RowType>{
       RowType("Steve", 12),
       RowType("Ann", 42),
   };
   int row_number = 0;
-  for (auto& row : result->Rows<RowType>()) {
+  for (auto& row : result.Rows<RowType>()) {
     EXPECT_STATUS_OK(row);
     EXPECT_EQ(*row, expected[row_number]);
     ++row_number;
@@ -143,21 +140,18 @@ TEST(ClientTest, ReadFailure) {
       )pb",
       &metadata));
   EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
-  EXPECT_CALL(*source, Stats())
-      .WillRepeatedly(Return(optional<spanner_proto::ResultSetStats>()));
   EXPECT_CALL(*source, NextValue())
       .WillOnce(Return(optional<Value>("Steve")))
       .WillOnce(Return(optional<Value>("Ann")))
       .WillOnce(Return(Status(StatusCode::kDeadlineExceeded, "deadline!")));
 
-  ResultSet result_set(std::move(source));
+  ReadResult result_set(std::move(source));
   EXPECT_CALL(*conn, Read(_)).WillOnce(Return(ByMove(std::move(result_set))));
 
   KeySet keys = KeySet::All();
   auto result = client.Read("table", std::move(keys), {"column1"});
-  EXPECT_STATUS_OK(result);
 
-  auto rows = result->Rows<Row<std::string>>();
+  auto rows = result.Rows<Row<std::string>>();
   auto iter = rows.begin();
   EXPECT_NE(iter, rows.end());
   EXPECT_STATUS_OK(*iter);
@@ -194,8 +188,6 @@ TEST(ClientTest, ExecuteQuerySuccess) {
       )pb",
       &metadata));
   EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
-  EXPECT_CALL(*source, Stats())
-      .WillRepeatedly(Return(optional<spanner_proto::ResultSetStats>()));
   EXPECT_CALL(*source, NextValue())
       .WillOnce(Return(optional<Value>("Steve")))
       .WillOnce(Return(optional<Value>(12)))
@@ -241,8 +233,6 @@ TEST(ClientTest, ExecuteQueryFailure) {
       )pb",
       &metadata));
   EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
-  EXPECT_CALL(*source, Stats())
-      .WillRepeatedly(Return(optional<spanner_proto::ResultSetStats>()));
   EXPECT_CALL(*source, NextValue())
       .WillOnce(Return(optional<Value>("Steve")))
       .WillOnce(Return(optional<Value>("Ann")))
@@ -329,8 +319,6 @@ TEST(ClientTest, ExecutePartitionedDml_Success) {
   auto source = make_unique<MockResultSetSource>();
   spanner_proto::ResultSetMetadata metadata;
   EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
-  EXPECT_CALL(*source, Stats())
-      .WillRepeatedly(Return(optional<spanner_proto::ResultSetStats>()));
   EXPECT_CALL(*source, NextValue()).WillRepeatedly(Return(optional<Value>()));
 
   std::string const sql_statement = "UPDATE Singers SET MarketingBudget = 1000";
@@ -418,17 +406,38 @@ TEST(ClientTest, RunTransactionCommit) {
   Transaction txn = MakeReadWriteTransaction();  // dummy
   Connection::ReadParams actual_read_params{txn, {}, {}, {}, {}};
   Connection::CommitParams actual_commit_params{txn, {}};
+
+  auto source = make_unique<MockResultSetSource>();
+  spanner_proto::ResultSetMetadata metadata;
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        row_type: {
+          fields: {
+            name: "Name",
+            type: { code: STRING }
+          }
+        }
+      )pb",
+      &metadata));
+  EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
+  EXPECT_CALL(*source, NextValue())
+      .WillOnce(Return(optional<Value>("Bob")))
+      .WillOnce(Return(optional<Value>()));
+  ReadResult result_set(std::move(source));
+
   EXPECT_CALL(*conn, Read(_))
-      .WillOnce(
-          DoAll(SaveArg<0>(&actual_read_params), Return(ByMove(ResultSet{}))));
+      .WillOnce(DoAll(SaveArg<0>(&actual_read_params),
+                      Return(ByMove(std::move(result_set)))));
   EXPECT_CALL(*conn, Commit(_))
       .WillOnce(DoAll(SaveArg<0>(&actual_commit_params),
                       Return(CommitResult{*timestamp})));
 
   auto mutation = MakeDeleteMutation("table", KeySet::All());
   auto f = [&mutation](Client client, Transaction txn) -> StatusOr<Mutations> {
     auto read = client.Read(std::move(txn), "T", KeySet::All(), {"C"});
-    if (!read) return read.status();
+    for (auto& row : read.Rows<Row<std::string>>()) {
+      if (!row) return row.status();
+    }
     return Mutations{mutation};
   };
 
@@ -447,16 +456,35 @@ TEST(ClientTest, RunTransactionRollback) {
   auto conn = std::make_shared<MockConnection>();
   Transaction txn = MakeReadWriteTransaction();  // dummy
   Connection::ReadParams actual_read_params{txn, {}, {}, {}, {}};
+
+  auto source = make_unique<MockResultSetSource>();
+  spanner_proto::ResultSetMetadata metadata;
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        row_type: {
+          fields: {
+            name: "Name",
+            type: { code: INT64 }
+          }
+        }
+      )pb",
+      &metadata));
+  EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
+  EXPECT_CALL(*source, NextValue())
+      .WillOnce(Return(Status(StatusCode::kInvalidArgument, "blah")));
+  ReadResult result_set(std::move(source));
+
   EXPECT_CALL(*conn, Read(_))
-      .WillOnce(
-          DoAll(SaveArg<0>(&actual_read_params),
-                Return(ByMove(Status(StatusCode::kInvalidArgument, "blah")))));
+      .WillOnce(DoAll(SaveArg<0>(&actual_read_params),
+                      Return(ByMove(std::move(result_set)))));
   EXPECT_CALL(*conn, Rollback(_)).WillOnce(Return(Status()));
 
   auto mutation = MakeDeleteMutation("table", KeySet::All());
   auto f = [&mutation](Client client, Transaction txn) -> StatusOr<Mutations> {
     auto read = client.Read(std::move(txn), "T", KeySet::All(), {"C"});
-    if (!read) return read.status();
+    for (auto& row : read.Rows<Row<std::string>>()) {
+      if (!row) return row.status();
+    }
     return Mutations{mutation};
   };
 
@@ -475,17 +503,36 @@ TEST(ClientTest, RunTransactionRollbackError) {
   auto conn = std::make_shared<MockConnection>();
   Transaction txn = MakeReadWriteTransaction();  // dummy
   Connection::ReadParams actual_read_params{txn, {}, {}, {}, {}};
+
+  auto source = make_unique<MockResultSetSource>();
+  spanner_proto::ResultSetMetadata metadata;
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        row_type: {
+          fields: {
+            name: "Name",
+            type: { code: INT64 }
+          }
+        }
+      )pb",
+      &metadata));
+  EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
+  EXPECT_CALL(*source, NextValue())
+      .WillOnce(Return(Status(StatusCode::kInvalidArgument, "blah")));
+  ReadResult result_set(std::move(source));
+
   EXPECT_CALL(*conn, Read(_))
-      .WillOnce(
-          DoAll(SaveArg<0>(&actual_read_params),
-                Return(ByMove(Status(StatusCode::kInvalidArgument, "blah")))));
+      .WillOnce(DoAll(SaveArg<0>(&actual_read_params),
+                      Return(ByMove(std::move(result_set)))));
   EXPECT_CALL(*conn, Rollback(_))
       .WillOnce(Return(Status(StatusCode::kInternal, "oops")));
 
   auto mutation = MakeDeleteMutation("table", KeySet::All());
   auto f = [&mutation](Client client, Transaction txn) -> StatusOr<Mutations> {
     auto read = client.Read(std::move(txn), "T", KeySet::All(), {"C"});
-    if (!read) return read.status();
+    for (auto& row : read.Rows<Row<std::string>>()) {
+      if (!row) return row.status();
+    }
     return Mutations{mutation};
   };
 
@@ -503,14 +550,33 @@ TEST(ClientTest, RunTransactionRollbackError) {
 #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
 TEST(ClientTest, RunTransactionException) {
   auto conn = std::make_shared<MockConnection>();
-  EXPECT_CALL(*conn, Read(_))
-      .WillOnce(Return(ByMove(Status(StatusCode::kInvalidArgument, "blah"))));
+
+  auto source = make_unique<MockResultSetSource>();
+  spanner_proto::ResultSetMetadata metadata;
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        row_type: {
+          fields: {
+            name: "Name",
+            type: { code: INT64 }
+          }
+        }
+      )pb",
+      &metadata));
+  EXPECT_CALL(*source, Metadata()).WillRepeatedly(Return(metadata));
+  EXPECT_CALL(*source, NextValue())
+      .WillOnce(Return(Status(StatusCode::kInvalidArgument, "blah")));
+  ReadResult result_set(std::move(source));
+
+  EXPECT_CALL(*conn, Read(_)).WillOnce(Return(ByMove(std::move(result_set))));
   EXPECT_CALL(*conn, Rollback(_)).WillOnce(Return(Status()));
 
   auto mutation = MakeDeleteMutation("table", KeySet::All());
   auto f = [&mutation](Client client, Transaction txn) -> StatusOr<Mutations> {
     auto read = client.Read(std::move(txn), "T", KeySet::All(), {"C"});
-    if (!read) throw "Read() error";
+    for (auto& row : read.Rows<Row<std::string>>()) {
+      if (!row) throw "Read() error";
+    }
     return Mutations{mutation};
   };
 

google/cloud/spanner/connection.h

@@ -133,7 +133,7 @@ class Connection {
   //@}
 
   /// Define the interface for a google.spanner.v1.Spanner.Read RPC
-  virtual StatusOr<ResultSet> Read(ReadParams) = 0;
+  virtual ReadResult Read(ReadParams) = 0;
 
   /// Define the interface for a google.spanner.v1.Spanner.PartitionRead RPC
   virtual StatusOr<std::vector<ReadPartition>> PartitionRead(