Skip to content

fix: Introspect to get column types in cursor.execute #200

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

Conversation

c24t
Copy link
Contributor

@c24t c24t commented Dec 22, 2020

Addresses googleapis/python-spanner-django#566.

This PR adds an extra call to get the table's schema before making inserts from cursor.execute. Spanner requires us to
pass parameter types to Transaction.execute_update, before this change we inferred the Spanner column types from the native python types in the prepared statement. Besides being a hack, this meant we couldn't handle inserts with null values. See googleapis/python-spanner-django#566 for more detail.

This change fixes many django test failures, notably the modeladmin and (most of) expressions_case tests, which all failed with InvalidArgument('Unable to infer type for parameter ....') before.

Note that Connection.run_statement still calls _execute_insert_heterogenous without param_types, and this still results in a call to get_param_types. I didn't replace this because it didn't affect the django tests I was running. If we pursue this change we should remove get_param_types altogether next.

There may be other problems with this approach that don't show up in the tests. For example, this change assumes the schema doesn't change between reading INFORMATION_SCHEMA.COLUMNS and doing the insert. Doing an extra read op before each insert may also slow the client down.

@c24t c24t requested a review from a team as a code owner December 22, 2020 06:18
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 22, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Dec 22, 2020
@c24t
Copy link
Contributor Author

c24t commented Dec 22, 2020

Note that Connection.run_statement still calls _execute_insert_heterogenous without param_types, and this still results in a call to get_param_types.

Running more of the django tests, it looks like this is responsible for another error:

TypeError: Parameter to MergeFrom() must be instance of same class: expected google.spanner.v1.Type got NoneType.

This shows up in model_inheritance among other tests.

self._itr = PeekIterator(self._result_set)
return

if classification == parse_utils.STMT_NON_UPDATING:
self._handle_DQL(sql, args or None)
elif classification == parse_utils.STMT_INSERT:
_helpers.handle_insert(self.connection, sql, args or None)

# Read INFORMATION_SCHEMA.COLUMNS to get the Spanner types for
Copy link
Contributor

Choose a reason for hiding this comment

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

I would strongly recommend that you only do this as a workaround when running against the emulator, as real Spanner seems to accept the parameter type to be missing. Otherwise you will be slowing down statements unnecessarily in production environments.
Furthermore, you probably need to do this for all types of statements on the emulator, not only insert statements.

@IlyaFaer
Copy link
Contributor

I suppose this PR can be closed. It adds some redundant code, which will be in production, but will work only for emulator cases. On SQLAlchemy we've just skipped the tests, which are not working on emulator, like this:
https://github.com/googleapis/python-spanner-sqlalchemy/blob/e97325de221d201a306ba92fecf8afc9856fa2c5/test/test_suite_13.py#L343-L345

I think let's do the same for Django. @olavloite, @larkee, what do you say?

@olavloite
Copy link
Contributor

I suppose this PR can be closed. It adds some redundant code, which will be in production, but will work only for emulator cases. On SQLAlchemy we've just skipped the tests, which are not working on emulator, like this: https://github.com/googleapis/python-spanner-sqlalchemy/blob/e97325de221d201a306ba92fecf8afc9856fa2c5/test/test_suite_13.py#L343-L345

I think let's do the same for Django. @olavloite, @larkee, what do you say?

Yes, this PR is more than a year old, and I would have serious doubts about the performance impacts of this. In addition, real Spanner supports non-typed parameters, and the missing support for non-typed parameters for some type (DATE and TIMESTAMP) in the emulator is considered a bug: GoogleCloudPlatform/cloud-spanner-emulator#31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. 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