Skip to content

fix: fix drop tables #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

HemangChothani
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 22, 2021
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will this allow us to remove the overrides relating to table clean up or are they still needed?

@HemangChothani
Copy link
Contributor Author

Still needed only for one thing, to delete the rows from the table before execute DROP TABLE like config.db.execute(int_table.delete())

Right now BooleanTest_spanner+spanner::test_render_literal_bool is failing due to the same reason, i think also need to override that method.

@IlyaFaer
Copy link
Contributor

@larkee, @HemangChothani, IIRC, on meeting we decided to fix it in Spanner connection API. As the Spanner DB API is the one who queues DDLs until anything else executed, the problem is actual for DB API as well. Fixing it in SQLAlchemy will not change it for Spanner connection API itself.

Here is the code, and @larkee already proposed a solution somewhere in our PRs (I don't remember where, but I do remember how).
https://github.com/googleapis/python-spanner/blob/1a7c9d296c23dfa7be7b07ea511a4a8fc2c0693f/google/cloud/spanner_dbapi/cursor.py#L175-L181

Thus, it's better be fixed in Spanner DB API to fix it globally.

@HemangChothani
Copy link
Contributor Author

@IlyaFaer Are you talking about #20 (comment) code? If yes, then alex suggested to modify the code and only apply for autocommit=True, which again started failing the tests and the code is #20 (comment).

@IlyaFaer
Copy link
Contributor

IlyaFaer commented Mar 24, 2021

@HemangChothani, @larkee, I was going to make an example to show you what is the problem, and it appears that there is even a dead end in autocommit mode. See the screenshot. I've tried to set autocommit mode and execute a DDL. Expectedly, it wasn't executed, only queued. And the thing is that I wasn't even able to do a commit - it's not working in autocommit mode. So, I have two ways in such a situation: set !autocommit mode back and do commit manually, or call run_prior_DDL_statements, which is weird in autocommit mode actually.

image

@IlyaFaer
Copy link
Contributor

@larkee, @HemangChothani, so, here is the problematic case. One inits a connection and executes a DDL in autocommit mode. Then one is closing a connection and creating a new connection, trying to continue the work. Expectedly, the DDL got lost somewhere in space, and the table wasn't created, though the DDL was executed in autocommit mode.

Безымянный

@HemangChothani
Copy link
Contributor Author

@larkee, @HemangChothani, so, here is the problematic case. One inits a connection and executes a DDL in autocommit mode. Then one is closing a connection and creating a new connection, trying to continue the work. Expectedly, the DDL got lost somewhere in space, and the table wasn't created, though the DDL was executed in autocommit mode.

Безымянный

@IlyaFaer Yes,I have also seen this issue many times when executing compliance tests.

@larkee
Copy link
Contributor

larkee commented Mar 24, 2021

Currently, the DDL statements are only executed if a non-DDL statement is executed after them. The fix for this is to call update_ddl immediately instead of queuing statements. @AVaksman pointed out that this would prevent batching of statements which would slow down workflows with a lot of DDL statements.

However, in light of the support to execute multiple DDL statements separated by ; I think the immediate update_ddl fix is the best option as it still allows users to batch statements together.

@HemangChothani
Copy link
Contributor Author

@larkee agreed, Immediate call of method self.database.update_ddl(ddl_statements).result() in both the conditions like autocommit= False or True will help to solve the issue.

@IlyaFaer IlyaFaer deleted the fix_drop_table_in_tests branch March 25, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants