Skip to content
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

Fix missing inferred types in insert statements #566

Closed
c24t opened this issue Dec 15, 2020 · 5 comments
Closed

Fix missing inferred types in insert statements #566

c24t opened this issue Dec 15, 2020 · 5 comments
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@c24t
Copy link
Contributor

c24t commented Dec 15, 2020

If we call transaction.execute_sql with missing parameters in param_types, we may not get an error until iterating over the over the ResultSet (even if it's empty). This errors is: InvalidArgument('Unable to infer type for parameter ....').

spanner_dbapi.parse_utils.get_param_types
infers spanner data types from the python types. This is called downstream of django's CursorWrapper.execute. Among other problems, this means that we can't infer the spanner type for None-valued fields, and so they aren't included in param_types.

This problem shows up following the django tutorial at manage.py createsuperuser, which results in these arguments to transaction.execute_sql:

sql="INSERT INTO auth_user (id, password, last_login, is_superuser, username, first_name, last_name, email, is_staff, is_active, date_joined) VALUES (@a0, @a1, @a2, @a3, @a4, @a5, @a6, @a7, @a8, @a9, @a10)"

params={'a0': 1451803748860449997,
 'a1': (hashed pwd),
 'a2': None,
 'a3': True,
 'a4': (username),
 'a5': '',
 'a6': '',
 'a7': '',
 'a8': True,
 'a9': True,
 'a10': '2020-12-14T23:57:50.577202Z'}

param_types={'a0': code: INT64,
 'a1': code: STRING,
 'a3': code: BOOL,
 'a4': code: STRING,
 'a5': code: STRING,
 'a6': code: STRING,
 'a7': code: STRING,
 'a8': code: BOOL,
 'a9': code: BOOL,
 'a10': code: TIMESTAMP}

Note that a2 is missing from param_types. The field last_login should be a google.cloud.spanner_v1.param_types.TIMESTAMP, but we can't infer it before the call.

Interestingly we only see the InvalidArgument error when using the emulator. The real non-emulated spanner service handles this insert without error.

This problem is at least as old as 47154d1, and existed in the spanner_dbapi package in this repo before it was moved into python-spanner.

It's not clear whether the right fix for this problem is here, in python-spanner, or in the emulator. But it does prevent us from using the emulator with apps that have models with nullable fields, including the django tutorial app.

@c24t c24t added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Dec 15, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner-django API. label Dec 15, 2020
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Dec 20, 2020
@IlyaFaer
Copy link
Contributor

@c24t, this is the most widespread error in tests, it's raised in ~200 different cases. Seems like an emulator problem to me.
@olavloite, could you comment? Did you faced this problem in Java?

@olavloite
Copy link
Contributor

@olavloite, could you comment? Did you faced this problem in Java?

@IlyaFaer No, both the Java client and the JDBC driver require all parameters (including nulls) to be typed, so it can always include the type field in the parameters that are sent.

If real Spanner accepts this and the emulator does not, then technically this is a problem with the emulator. At the same time the emulator does not guarantee 100% compatibility with real Spanner, so it might be that this is something you will have to live with.

One option could be to try to include STRING as the default type code with untyped null parameters and see if both the emulator and real Spanner accept that.

@c24t
Copy link
Contributor Author

c24t commented Dec 22, 2020

One option could be to try to include STRING as the default type code with untyped null parameters and see if both the emulator and real Spanner accept that.

I gave this a shot in c24t/python-spanner@e339d6a, but it fails (as you might expect) for non-STRING-valued columns.

ERROR: test_update_without_default (expressions_case.tests.CaseExpressionTests)
...
File ".../google-cloud-spanner/google/cloud/spanner_dbapi/cursor.py", line 207, in execute
    raise ProgrammingError(e.details if hasattr(e, "details") else e)
django.db.utils.ProgrammingError: 400 Value has type STRING which cannot be inserted into column big_integer, which has type INT64 [at 1:376]
...url, uuid, fk_id) VALUES (@a0, @a1, @a2, @a3, @a4, @a5, @a6, @a7, @a8, @a9...

There are a few django tests where all the uninferable null fields are actually STRING-like -- like modeladmin -- but this fails for other tests like expression_case.

I suspect there's a better way to do this, one that doesn't rely on inferring the parameter types at all.

@c24t
Copy link
Contributor Author

c24t commented Dec 22, 2020

One approach to fix this in googleapis/python-spanner#200.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Dec 22, 2020
@c24t c24t added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Jan 12, 2021
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jan 12, 2021
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Apr 12, 2021
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 14, 2021
@asthamohta
Copy link
Contributor

Adding support to this would have serious 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
Further details about the decisions can be checked here: googleapis/python-spanner#200

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-django API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants