feat: avoid adding dummy WHERE clause into UPDATE and DELETE queires #516
Conversation
IlyaFaer
added
type: feature request
google-cla
bot
added
the
cla: yes
|
@c24t, looks like it's our way to raise exceptions on lack of WHERE statement, but not to break all the Django tests. What do you think? |
IlyaFaer
removed
the
do not merge
mf2199
added
the
kokoro:force-run
| "ALL", | ||
| "AND", | ||
| "ANY", | ||
| "ARRAY", | ||
| "AS", | ||
| "ASC", | ||
| "ASSERT_ROWS_MODIFIED", | ||
| "AT", | ||
| "BETWEEN", | ||
| "BY", | ||
| "CASE", | ||
| "CAST", | ||
| "COLLATE", | ||
| "CONTAINS", | ||
| "CREATE", | ||
| "CROSS", | ||
| "CUBE", | ||
| "CURRENT", | ||
| "DEFAULT", | ||
| "DEFINE", | ||
| "DESC", | ||
| "DISTINCT", | ||
| "DROP", | ||
| "ELSE", | ||
| "END", | ||
| "ENUM", | ||
| "ESCAPE", | ||
| "EXCEPT", | ||
| "EXCLUDE", | ||
| "EXISTS", | ||
| "EXTRACT", | ||
| "FALSE", | ||
| "FETCH", | ||
| "FOLLOWING", | ||
| "FOR", | ||
| "FROM", | ||
| "FULL", | ||
| "GROUP", | ||
| "GROUPING", | ||
| "GROUPS", | ||
| "HASH", | ||
| "HAVING", | ||
| "IF", | ||
| "IGNORE", | ||
| "IN", | ||
| "INNER", | ||
| "INTERSECT", | ||
| "INTERVAL", | ||
| "INTO", | ||
| "IS", | ||
| "JOIN", | ||
| "LATERAL", | ||
| "LEFT", | ||
| "LIKE", | ||
| "LIMIT", | ||
| "LOOKUP", | ||
| "MERGE", | ||
| "NATURAL", | ||
| "NEW", | ||
| "NO", | ||
| "NOT", | ||
| "NULL", | ||
| "NULLS", | ||
| "OF", | ||
| "ON", | ||
| "OR", | ||
| "ORDER", | ||
| "OUTER", | ||
| "OVER", | ||
| "PARTITION", | ||
| "PRECEDING", | ||
| "PROTO", | ||
| "RANGE", | ||
| "RECURSIVE", | ||
| "RESPECT", | ||
| "RIGHT", | ||
| "ROLLUP", | ||
| "ROWS", | ||
| "SELECT", | ||
| "SET", | ||
| "SOME", | ||
| "STRUCT", | ||
| "TABLESAMPLE", | ||
| "THEN", | ||
| "TO", | ||
| "TREAT", | ||
| "TRUE", | ||
| "UNBOUNDED", | ||
| "UNION", | ||
| "UNNEST", | ||
| "USING", | ||
| "WHEN", | ||
| "WHERE", | ||
| "WINDOW", | ||
| "WITH", | ||
| "WITHIN", | ||
| } | ||
| raise ProgrammingError( | ||
| "Cloud Spanner requires a WHERE clause when executing DELETE or UPDATE query" | ||
| ) |
There was a problem hiding this comment.
Apologies if I misunderstood something here.
I believe this clause is added by default because Django will construct DELETE and UPDATE queries that don't have the WHERE clause as this is not a requirement in other databases. It ensure that customers can smoothly transition from their existing applications without having to write raw queries with the WHERE clause.
The clause wasn't added just to get the tests to pass.
There was a problem hiding this comment.
@skuruppu, hiding this useful restriction doesn't look right. But I see the point in smooth transition as well. Do you mean that only for Django? Or we should consider that any user can have queries without WHERE, so this dummy should be kept for the whole connection API? (we have spanner_dbapi directory, which seems to be intended to be default connection API - it's from where I'm proposing to erase the dummy, and django_spanner directory, where Django-specific parts are held).
The main thing that makes me nervous is that queries can be big and branched (for example, UNIONS are little bit hard to interpret, as WHERE clause can be relative to the last of the union parts, but also can be relative to the result of the UNION on the whole, and they both will be at the about same place). I'm not sure if there is a way to check 100% of variants with several regexes, and insert this dummy correctly. So, I think it'll be exploding things from time to time.
There was a problem hiding this comment.
I agree that it may be good to notify the user about this restriction from the connection API. Is it possible to instead modify the Django implementation to add the WHERE clause in the generated query? I think this is where it matters the most in order to support users migrating from other databases.
mf2199
added
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
Talking about this PR in person: following @skuruppu's comment, these changes should move to django-specific code. |
…c/python-spanner-django into one_more_where_clause_try
|
@c24t, I've pushed the changes, it seems to be ready for merge now. |
There was a problem hiding this comment.
LGTM, but please rename one of those ensure_where_clause funcs.
Note that this will have to be split up across python-spanner and spanner-django now because of googleapis/python-spanner#160.
|
@IlyaFaer just pushed some minor fixes for my own comments, if it LGTY we can merge. |
|
@c24t, yeah, I'm okay with this changes |
|
Note that we need to rip out |
No description provided.