feat: Implementation for batch dml in dbapi by ankiaga · Pull Request #1055 · googleapis/python-spanner · GitHub

No description provided.

nit: can this be just one string instead of two concatenated strings?

Fixed

Should (could) this also include the status code?

Will take it in a follow up PR

If I understand it correctly, giving these names that do not start with an underscore will make them part of the public API. In that case, we should document them and also add validations to verify that they are only called with valid arguments. (But probably we should make them internal instead)

Removed the property

What happens if an external uses calls this function when the connection is already in the middle of a different type of batch (e.g. it is now in a DML batch, and it is called to set it to DDL batch)?

Removed this property

It's probably difficult, but: Do we have any way that we could verify that the run batch call really sends an ExecuteBatchDml request, and does not execute multiple ExecuteSql requests sequentially?

Do you mean if we could assert something in the test for that?

Manually I have verified it by debugging

Yes, I meant automated. That would then also guard us against any regressions, for example if something changes in the underlying client library, which cause this to use multiple ExecuteSql RPCs instead of one ExecuteBatchDml RPC.

This needs some investigation, so will take it in a follow up PR

We should also have tests for the unhappy path of using batch DML:

What happens if the batch contains an invalid statement as the first statement?
What happens if the batch contains an invalid statement in the middle?
What happens if the batch contains an invalid statement at the end?
Can we test and verify that the retry logic for Batch DML works as expected?

Added test for invalid statements.

There are issues with the retry logic which I am fixing in the next PR

Ack

Looks generally good to me, but with one remaining question/nit on the exception that is raised when a batch fails. The exception should also contain the update counts of the statements that did succeed. Are we sure that it does?

Yes, I meant automated. That would then also guard us against any regressions, for example if something changes in the underlying client library, which cause this to use multiple ExecuteSql RPCs instead of one ExecuteBatchDml RPC.

I would have expected this to return a specific batch error that:

Contains the error code, message etc.
And contains the update counts of the statements that did succeed.

See for example this for the JDBC driver: https://github.com/googleapis/java-spanner-jdbc/blob/1f89f78c37b9e118e2c0cbc7f56d3eb1d5745863/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcPreparedStatementTest.java#L797

As discussed we need to create a custom exception class, so will take it in follow up PR

Why did this change in this PR?

The logic to add WHERE clause has moved to parse_utils.classify_statement now which we are mocking here so its same as what is returned as per line 255

Ack

feat: Implementation for batch dml in dbapi

bb19174

ankiaga requested review from a team December 12, 2023 03:40

product-auto-label Bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/python-spanner API. labels Dec 12, 2023

ankiaga requested review from aseering and olavloite December 12, 2023 03:40

Few changes

3ceb081

ankiaga added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 12, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 12, 2023

ankiaga requested review from manu2 and pratickchokhani December 12, 2023 07:53

olavloite reviewed Dec 13, 2023

View reviewed changes

Incorporated comments

6ff121d

ankiaga added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2023

ankiaga added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 14, 2023

olavloite approved these changes Dec 14, 2023

View reviewed changes

Merge branch 'main' into batch_dml

2157ebf

ankiaga enabled auto-merge (squash) December 14, 2023 17:17

ankiaga merged commit 7a92315 into googleapis:main Dec 14, 2023

release-please Bot mentioned this pull request Dec 14, 2023

chore(main): release 3.41.0 #1009

Merged

+                          and parsed_statement.statement_type != StatementType.INSERT
+                      ):
+                          raise ProgrammingError(
+                              "Only DML statements are allowed in batch " "DML mode."

+                                  connection._transaction = None
+                                  raise Aborted(status.message)
+                              elif status.code != OK:
+                                  raise OperationalError(status.message)

+                      Args:
+                          value (BatchMode)
+                      """
+                      self._batch_mode = value

+                      self._cursor.execute("start batch dml")
+                      self._insert_row(7)
+                      self._insert_row(8)
+                      self._cursor.execute("run batch")

+                      self._cursor.execute("start batch dml")
+                      self._insert_row(7)
+                      self._insert_row(8)
+                      self._cursor.execute("run batch")

                       self._read_only = value
+                  @property
+                  def batch_mode(self):

+                          VALUES ({i}, 'first-name-{i}', 'last-name-{i}', 'test.email@domen.ru')
+                          """
+                      )

                           mock_db.run_in_transaction = mock_run_in = mock.MagicMock()
                           cursor.execute(sql="sql")
                           mock_run_in.assert_called_once_with(
-                              cursor._do_execute_update_in_autocommit, "sql WHERE 1=1", None

feat: Implementation for batch dml in dbapi by ankiaga · Pull Request #1055 · googleapis/python-spanner · GitHub

Uh oh!

feat: Implementation for batch dml in dbapi - #1055

feat: Implementation for batch dml in dbapi#1055ankiaga merged 4 commits intogoogleapis:maingoogleapis/python-spanner:mainfrom ankiaga:batch_dmlankiaga/python-spanner:batch_dmlCopy head branch name to clipboard

Conversation

ankiaga commented Dec 12, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankiaga Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: Implementation for batch dml in dbapi#1055
ankiaga merged 4 commits into
googleapis:mainfrom
ankiaga:batch_dml

ankiaga Dec 14, 2023 •
edited
Loading