fix(db_api): ensure DDL statements are being executed - #290
larkee merged 3 commits into
Conversation
Sorry, something went wrong.
| ddl = ddl.strip() | ||
| if ddl: | ||
| self.connection._ddl_statements.append(ddl) | ||
| if self.connection.autocommit: |
There was a problem hiding this comment.
I think we should execute the statements immediately regardless of whether autocommit is set or not. Because Spanner handles DDL separately from transactions, there's no way to roll back DDL statements (which we will need to clearly document) and so I think having different behavior based on autocommit is unnecessary.
@AVaksman WDYT? Note that we now support executing multiple DDL statements at once which should alleviate the batching concern.
Sorry, something went wrong.
There was a problem hiding this comment.
@larkee, no problem, I'll delete the line.
I'm writing a system test to check the case.
Also I've found out that in !autocommit mode DDLs are also not executed, even if one will call commit(). Will fix it in this PR as well.
Sorry, something went wrong.
There was a problem hiding this comment.
@larkee, you know, about regarding autocommit mode... I think users don't usually go deep into frameworks code. So, we support DDLs separated with ;, still users may use frameworks, which are generating and executing DDLs one by one. In this case it's probably better to look after autocommit mode, to make DDLs pumping up in !autocommit mode - to execute them all at once little bit later.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, that's a good point. In that case, we'll need to add run_prior_DDL_statements() to commit() as currently the statements aren't being executed even when commit() is called.
Sorry, something went wrong.
|
@larkee, something failed in system tests, but it doesn't seem to be related, as DB API system tests live in another file: __________ TestSessionAPI.test_transaction_batch_update_w_parent_span __________
self = <tests.system.test_system.TestSessionAPI testMethod=test_transaction_batch_update_w_parent_span>
def test_transaction_batch_update_w_parent_span(self):
try:
from opentelemetry import trace
except ImportError:
return
tracer = trace.get_tracer(__name__)
retry = RetryInstanceState(_has_all_ddl)
retry(self._db.reload)()
session = self._db.session()
session.create()
self.to_delete.append(session)
with session.batch() as batch:
batch.delete(self.TABLE, self.ALL)
insert_statement = list(self._generate_insert_statements())[0]
update_statement = (
"UPDATE contacts SET email = @email " "WHERE contact_id = @contact_id;",
{"contact_id": 1, "email": "phreddy@example.com"},
{"contact_id": param_types.INT64, "email": param_types.STRING},
)
delete_statement = (
"DELETE contacts WHERE contact_id = @contact_id;",
{"contact_id": 1},
{"contact_id": param_types.INT64},
)
def unit_of_work(transaction, self):
status, row_counts = transaction.batch_update(
[insert_statement, update_statement, delete_statement]
)
self._check_batch_status(status.code)
self.assertEqual(len(row_counts), 3)
for row_count in row_counts:
self.assertEqual(row_count, 1)
with tracer.start_as_current_span("Test Span"):
session.run_in_transaction(unit_of_work, self)
span_list = self.memory_exporter.get_finished_spans()
> self.assertEqual(len(span_list), 6)
E AssertionError: 9 != 6
tests/system/test_system.py:1774: AssertionError
|
Sorry, something went wrong.
|
This is a known flaky test that I have an issue for: #165 I'll force the tests to rerun 👍 |
Sorry, something went wrong.
larkee
left a comment
There was a problem hiding this comment.
LGTM, just some comments on the system tests
Sorry, something went wrong.
|
@larkee, checks passed. They stuck once for 5 hours, so I've reexecuted them |
Sorry, something went wrong.
🤖 I have created a release \*beep\* \*boop\* --- ## [3.3.0](https://www.github.com/googleapis/python-spanner/compare/v3.2.0...v3.3.0) (2021-03-25) ### Features * add encryption_info to Database ([#284](https://www.github.com/googleapis/python-spanner/issues/284)) ([2fd0352](https://www.github.com/googleapis/python-spanner/commit/2fd0352f695d7ab85e57d8c4388f42f91cf39435)) * add support for CMEK ([#105](https://www.github.com/googleapis/python-spanner/issues/105)) ([e990ff7](https://www.github.com/googleapis/python-spanner/commit/e990ff70342e7c2e27059e82c8d74cce39eb85d0)) * add support for custom timeout and retry parameters in execute_update method in transactions ([#251](https://www.github.com/googleapis/python-spanner/issues/251)) ([8abaebd](https://www.github.com/googleapis/python-spanner/commit/8abaebd9edac198596e7bd51d068d50147d0391d)) * added retry and timeout params to partition read in database and snapshot class ([#278](https://www.github.com/googleapis/python-spanner/issues/278)) ([1a7c9d2](https://www.github.com/googleapis/python-spanner/commit/1a7c9d296c23dfa7be7b07ea511a4a8fc2c0693f)) * **db_api:** support executing several DDLs separated by semicolon ([#277](https://www.github.com/googleapis/python-spanner/issues/277)) ([801ddc8](https://www.github.com/googleapis/python-spanner/commit/801ddc87434ff9e3c86b1281ebfeac26195c06e8)) ### Bug Fixes * avoid consuming pending null values when merging ([#286](https://www.github.com/googleapis/python-spanner/issues/286)) ([c6cba9f](https://www.github.com/googleapis/python-spanner/commit/c6cba9fbe4c717f1f8e2a97e3f76bfe6b956e55b)) * **db_api:** allow file path for credentials ([#221](https://www.github.com/googleapis/python-spanner/issues/221)) ([1de0284](https://www.github.com/googleapis/python-spanner/commit/1de028430b779a50d38242fe70567e92b560df5a)) * **db_api:** ensure DDL statements are being executed ([#290](https://www.github.com/googleapis/python-spanner/issues/290)) ([baa02ee](https://www.github.com/googleapis/python-spanner/commit/baa02ee1a352f7c509a3e169927cf220913e521f)) * **db_api:** revert Mutations API usage ([#285](https://www.github.com/googleapis/python-spanner/issues/285)) ([e5d4901](https://www.github.com/googleapis/python-spanner/commit/e5d4901e9b7111b39dfec4c56032875dc7c6e74c)) ### Documentation * fix docstring types and typos ([#259](https://www.github.com/googleapis/python-spanner/issues/259)) ([1b0ce1d](https://www.github.com/googleapis/python-spanner/commit/1b0ce1d2986085ce4033cf773eb6c5d3b904473c)) * fix snapshot usage ([#291](https://www.github.com/googleapis/python-spanner/issues/291)) ([eee2181](https://www.github.com/googleapis/python-spanner/commit/eee218164c3177586b73278aa21495280984af89)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR ensures that DDL statements are being executed.
Currently, DDL statements aren't executed until a non-DDL statement is executed. This leads to unexpected behavior. This fix will execute DDL statements immediately when autocommit=True and when commit() is called for autocommit=False.
Note that DDL statements will still be executed when a non-DDL statements is executed in autocommit=False. e.g.
cur.execute( """ CREATE TABLE Singers ( SingerId INT64 NOT NULL, Name STRING(1024), ) PRIMARY KEY (SingerId) """ ) # CREATE TABLE statement will be execute here; before the INSERT statement. cur.execute( """ INSERT INTO Singers (SingerId, Name) VALUES (1, "Name") """ )