feat: Rerouted DirectRow.commit to use MutateRow #1276

daniel-sanche · 2026-01-27T23:55:36Z

The docstring says "If no mutations have been created in the row, no request is made.". But it looks like mutate_row raises a ValueError if no mutations are present.

You should probably catch and swallow that exception, and make sure this case is handled in tests

daniel-sanche · 2026-01-27T23:56:38Z

mutate_row has automatic retries. Is that expected? Did the previous implementation retry?

The previous implementation used Table.mutate_rows with the default retry.

daniel-sanche · 2026-01-27T23:48:58Z

Conisder this for clarity

Suggested change

return status_pb2.Status()

return status_pb2.Status(code_pb2.OK)

gemini-code-assist · 2026-01-26T16:33:33Z

The DirectRow.commit method now relies on the underlying _table_impl.mutate_row to validate the number of mutations and handle empty mutation lists. This is a change from the previous client-side validation for MAX_MUTATIONS. However, ConditionalRow.commit (lines 591-602 in the full file) still performs client-side checks for MAX_MUTATIONS and returns early for empty mutation lists. This inconsistency in validation strategy across Row subclasses could lead to varied error behaviors and should be addressed for better maintainability and predictability. Consider aligning the validation approach for ConditionalRow.commit with DirectRow.commit, or reintroducing explicit client-side validation in DirectRow.commit if that is the desired behavior for all Row types.

gemini-code-assist · 2026-01-26T16:33:33Z

The comment on line 1095 states, "Not having any mutations gives a ValueError enforced on the client side." However, the DirectRow.commit method in google/cloud/bigtable/row.py does not explicitly contain this validation logic; it relies on the underlying _table_impl.mutate_row call to raise an error for empty mutations. To ensure the comment accurately reflects the code's behavior, either add explicit client-side validation for empty mutations within DirectRow.commit (for consistency with ConditionalRow.commit), or update this comment to clarify that the ValueError originates from the underlying API client.

-Original file line number
+Diff line change
@@ Expand Up / @@ -219,6 +219,37 @@ def test_table_read_rows_filter_millis(data_table): @@
         row_data.consume_all()
+    def test_table_direct_row_commit(data_table, rows_to_delete):
+        from google.rpc import code_pb2
+        row = data_table.direct_row(ROW_KEY)
+        # Test set cell
+        row.set_cell(COLUMN_FAMILY_ID1, COL_NAME1, CELL_VAL1)
+        row.set_cell(COLUMN_FAMILY_ID1, COL_NAME2, CELL_VAL1)
+        status = row.commit()
+        rows_to_delete.append(row)
+        assert status.code == code_pb2.Code.OK
+        row_data = data_table.read_row(ROW_KEY)
+        assert row_data.cells[COLUMN_FAMILY_ID1][COL_NAME1][0].value == CELL_VAL1
+        assert row_data.cells[COLUMN_FAMILY_ID1][COL_NAME2][0].value == CELL_VAL1
+        # Test delete cell
+        row.delete_cell(COLUMN_FAMILY_ID1, COL_NAME1)
+        status = row.commit()
+        assert status.code == code_pb2.Code.OK
+        row_data = data_table.read_row(ROW_KEY)
+        assert COL_NAME1 not in row_data.cells[COLUMN_FAMILY_ID1]
+        assert row_data.cells[COLUMN_FAMILY_ID1][COL_NAME2][0].value == CELL_VAL1
+        # Test delete row
+        row.delete()
+        status = row.commit()
+        assert status.code == code_pb2.Code.OK
+        row_data = data_table.read_row(ROW_KEY)
+        assert row_data is None
     def test_table_mutate_rows(data_table, rows_to_delete):
         row1 = data_table.direct_row(ROW_KEY)
         row1.set_cell(COLUMN_FAMILY_ID1, COL_NAME1, CELL_VAL1)
@@ Expand Down Expand Up @@
     def test_table_direct_row_input_errors(data_table, rows_to_delete):
-        from google.api_core.exceptions import InvalidArgument
         from google.cloud.bigtable.row import MAX_MUTATIONS
         row = data_table.direct_row(ROW_KEY)
@@ Expand All @@
         with pytest.raises(TypeError):
             row.set_cell(COLUMN_FAMILY_ID1, COL_NAME1, FLOAT_CELL_VAL)
-        # Can't have more than MAX_MUTATIONS mutations, but only enforced after
-        # a row.commit
+        # Can't have more than MAX_MUTATIONS mutations, enforced on server side now.
         row.clear()
         for _ in range(0, MAX_MUTATIONS + 1):
             row.set_cell(COLUMN_FAMILY_ID1, COL_NAME1, CELL_VAL1)
-        with pytest.raises(ValueError):
-            row.commit()
+        resp = row.commit()
+        assert resp.code == StatusCode.INVALID_ARGUMENT.value[0]
-        # Not having any mutations gives a server error (InvalidArgument), not
-        # enforced on the client side.
+        # Not having any mutations gives a ValueError enforced on the client side.
         row.clear()
-        with pytest.raises(InvalidArgument):
+        with pytest.raises(ValueError):
             row.commit()
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -368,44 +368,110 @@ def test_direct_row_delete_cells_with_string_columns(): @@
     def test_direct_row_commit():
+        from google.cloud.bigtable_v2.services.bigtable import BigtableClient
+        from google.rpc import code_pb2, status_pb2
         project_id = "project-id"
         row_key = b"row_key"
         table_name = "projects/more-stuff"
+        app_profile_id = "app_profile_id"
         column_family_id = "column_family_id"
         column = b"column"
         credentials = _make_credentials()
         client = _make_client(project=project_id, credentials=credentials, admin=True)
-        table = _Table(table_name, client=client)
+        table = _Table(table_name, client=client, app_profile_id=app_profile_id)
         row = _make_direct_row(row_key, table)
         value = b"bytes-value"
+        # Set mock
+        api = mock.create_autospec(BigtableClient)
+        response_pb = _MutateRowResponsePB()
+        api.mutate_row.side_effect = [response_pb]
+        client.table_data_client
+        client._table_data_client._gapic_client = api
         # Perform the method and check the result.
         row.set_cell(column_family_id, column, value)
-        row.commit()
-        assert table.mutated_rows == [row]
+        response = row.commit()
+        assert row._mutations == []
+        assert response == status_pb2.Status(code=code_pb2.OK)
+        call_args = api.mutate_row.call_args
+        assert app_profile_id == call_args.app_profile_id[0]
     def test_direct_row_commit_with_exception():
-        from google.rpc import status_pb2
+        from google.api_core.exceptions import InternalServerError
+        from google.cloud.bigtable_v2.services.bigtable import BigtableClient
+        from google.rpc import code_pb2, status_pb2
         project_id = "project-id"
         row_key = b"row_key"
         table_name = "projects/more-stuff"
+        app_profile_id = "app_profile_id"
         column_family_id = "column_family_id"
         column = b"column"
         credentials = _make_credentials()
         client = _make_client(project=project_id, credentials=credentials, admin=True)
-        table = _Table(table_name, client=client)
+        table = _Table(table_name, client=client, app_profile_id=app_profile_id)
         row = _make_direct_row(row_key, table)
         value = b"bytes-value"
+        # Set mock
+        api = mock.create_autospec(BigtableClient)
+        exception_message = "Boom!"
+        exception = InternalServerError(exception_message)
+        api.mutate_row.side_effect = [exception]
+        client.table_data_client
+        client._table_data_client._gapic_client = api
         # Perform the method and check the result.
         row.set_cell(column_family_id, column, value)
         result = row.commit()
-        expected = status_pb2.Status(code=0)
-        assert result == expected
+        assert row._mutations == []
+        assert result == status_pb2.Status(
+            code=code_pb2.Code.INTERNAL, message=exception_message
+        )
+        call_args = api.mutate_row.call_args
+        assert app_profile_id == call_args.app_profile_id[0]
+    def test_direct_row_commit_with_unknown_exception():
+        from google.api_core.exceptions import GoogleAPICallError
+        from google.cloud.bigtable_v2.services.bigtable import BigtableClient
+        from google.rpc import code_pb2, status_pb2
+        project_id = "project-id"
+        row_key = b"row_key"
+        table_name = "projects/more-stuff"
+        app_profile_id = "app_profile_id"
+        column_family_id = "column_family_id"
+        column = b"column"
+        credentials = _make_credentials()
+        client = _make_client(project=project_id, credentials=credentials, admin=True)
+        table = _Table(table_name, client=client, app_profile_id=app_profile_id)
+        row = _make_direct_row(row_key, table)
+        value = b"bytes-value"
+        # Set mock
+        api = mock.create_autospec(BigtableClient)
+        exception_message = "Boom!"
+        exception = GoogleAPICallError(message=exception_message)
+        api.mutate_row.side_effect = [exception]
+        client.table_data_client
+        client._table_data_client._gapic_client = api
+        # Perform the method and check the result.
+        row.set_cell(column_family_id, column, value)
+        result = row.commit()
+        assert row._mutations == []
+        assert result == status_pb2.Status(
+            code=code_pb2.Code.UNKNOWN, message=exception_message
+        )
+        call_args = api.mutate_row.call_args
+        assert app_profile_id == call_args.app_profile_id[0]
     def _make_conditional_row(*args, **kwargs):
@@ Expand Down Expand Up / @@ -729,6 +795,12 @@ def test__parse_rmw_row_response(): @@
         assert expected_output == _parse_rmw_row_response(sample_input)
+    def _MutateRowResponsePB():
+        from google.cloud.bigtable_v2.types import bigtable as messages_v2_pb2
+        return messages_v2_pb2.MutateRowResponse()
     def _CheckAndMutateRowResponsePB(*args, **kw):
         from google.cloud.bigtable_v2.types import bigtable as messages_v2_pb2
@@ Expand Down Expand Up @@
             self._instance = _Instance(client)
             self._app_profile_id = app_profile_id
             self.client = client
-            self.mutated_rows = []
             self._table_impl = self._instance._client._veneer_data_client.get_table(
                 _INSTANCE_ID,
                 self.name,
                 app_profile_id=self._app_profile_id,
             )
-        def mutate_rows(self, rows):
-            from google.rpc import status_pb2
-            self.mutated_rows.extend(rows)
-            return [status_pb2.Status(code=0)]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rerouted DirectRow.commit to use MutateRow #1276

Uh oh!

Diff view

Diff view

There are no files selected for viewing

daniel-sanche Jan 27, 2026

Uh oh!

daniel-sanche Jan 27, 2026

Uh oh!

gkevinzheng Jan 28, 2026

Uh oh!

daniel-sanche Jan 27, 2026

Uh oh!

gemini-code-assist bot Jan 26, 2026

Uh oh!

gemini-code-assist bot Jan 26, 2026

Uh oh!

Uh oh!

feat: Rerouted DirectRow.commit to use MutateRow #1276

Are you sure you want to change the base?

Uh oh!

feat: Rerouted DirectRow.commit to use MutateRow #1276

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

daniel-sanche Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

gkevinzheng Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!