-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Rewrite the user_filter migration again #6184
Conversation
you can't plausibly ALTER TABLE in sqlite, so we create the new table with the right schema to start with.
""" | ||
else: | ||
select_clause = """ | ||
CREATE TEMPORARY TABLE user_filters_migration AS | ||
SELECT * FROM user_filters GROUP BY user_id, filter_id; | ||
SELECT * FROM user_filters GROUP BY user_id, filter_id | ||
""" | ||
sql = ( | ||
""" | ||
BEGIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're already in a transaction? Which would mean that the END
would commit everything in the current transaction, which is probably not what we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, hum. maybe? I don't think it's useful, anyway. will remove.
""" | ||
else: | ||
select_clause = """ | ||
CREATE TEMPORARY TABLE user_filters_migration AS | ||
SELECT * FROM user_filters GROUP BY user_id, filter_id; | ||
SELECT * FROM user_filters GROUP BY user_id, filter_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this work for both postgres and sqlite now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, postgres complains because filter_json
is neither in the group by nor explicitly aggregated. I considered futzing around with something to pick an arbitrary filter_json from the results, but in the end decided to stick with pik's solution.
you can't plausibly ALTER TABLE in sqlite, so we create the new table with the
right schema to start with.