feat: Support property group migrations on distributed and sharded tables #24274
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… writable (sharded) tables
tkaemming
changed the title
tkaemming
commented
Aug 8, 2024
|
|
||
| def get_create_table_pieces(self, table: TableName) -> Iterable[str]: |
There was a problem hiding this comment.
Not sure what the correct terminology is here. They're not expressions, but also not whole statements, so just went with something generic.
Comment on lines
+16
to
+18
| TableName = str | ||
| ColumnName = str | ||
| PropertyGroupName = str |
There was a problem hiding this comment.
(These aliases are just for documentation purposes.)
| "sharded_events": event_property_group_definitions, | ||
| "events": { | ||
| column_name: { | ||
| group_name: dataclasses.replace(group_definition, is_materialized=False) |
There was a problem hiding this comment.
This is kind of clunky — maybe in retrospect it would have been better to have this as some sort of flag up at the table level rather than each individual property group definition…
There was a problem hiding this comment.
yeah it's definitely not easy reading
fuziontech
reviewed
Aug 12, 2024
Comment on lines
+62
to
+65
| for group_name in groups: | ||
| yield self.__get_column_definition(table, column, group_name) | ||
| for index_definition in self.__get_index_definitions(table, column, group_name): | ||
| yield f"INDEX {index_definition}" |
There was a problem hiding this comment.
I dig this. Really nice use of python iterator building up table bits
fuziontech
added a commit
that referenced
this pull request
Aug 13, 2024
Co-authored-by: James Greenhill <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
#24152 added the property group columns to the sharded tables, but not the distributed tables.
Changes
This extracts the migration related parts from #24171:
PropertyGroupManagerto more cleanly support managing multiple tablessharded_eventstable in migrationposthog/models/event/sql.pyso that they are created in test environment which does not apply migrations, and updates migrations to useIF NOT EXISTSto avoid conflictsDoes this work well for both Cloud and self-hosted?
Yes.
How did you test this code?
Run migrations and see updated snapshot.