feat: Activate Version with example test #89
Conversation
| | sqlalchemy_url | False | None | SQLAlchemy connection string. This will override using host, user, password, port,dialect. Note that you must esacpe password specialcharacters properly seehttps://docs.sqlalchemy.org/en/20/core/engines.html#escaping-special-characters-such-as-signs-in-passwords | | ||
| | dialect+driver | False | postgresql+psycopg2 | Dialect+driver see https://docs.sqlalchemy.org/en/20/core/engines.html. Generally just leave this alone. Note if sqlalchemy_url is set this will be ignored. | | ||
| | default_target_schema | False | None | Postgres schema to send data to, example: tap-clickup | | ||
| | hard_delete | False | 0 | When activate version is sent from a tap this specefies if we should delete the records that don't match, or mark them with a date in the `_sdc_deleted_at` column. | |
There was a problem hiding this comment.
Boolean should be generated as True / False here I think? I'll leave for now
There was a problem hiding this comment.
Agreed. Very strange that it shows defaults of 0. Agreed though we don't need to block on this.
| @@ -252,3 +255,62 @@ def schema_name(self) -> Optional[str]: | |||
|
|
|||
| # Schema name not detected. | |||
| return None | |||
|
|
|||
| def activate_version(self, new_version: int) -> None: | |||
There was a problem hiding this comment.
I think we could take this function and just drop it into the SDK as it is here, this just has a few tweaks that are needed to make this work in a more generic way
target_postgres/sinks.py
Outdated
| if self.config["hard_delete"] is True: | ||
| self.connection.execute( | ||
| f"DELETE FROM {self.full_table_name} " | ||
| f"WHERE {self.version_column_name} <= {new_version} OR {self.version_column_name} IS NULL" |
There was a problem hiding this comment.
I'm a little worried about what would happen if someone ran this in production with add_record_metadata set to True for a while, then suddenly they change it to False.
I think we'd end up dropping the entire table, because we'd add all the new records, then none of them would have a new_version set they'd all be NULL. Then we'd drop all the NULLs. :O
Removing the IS NULL check would work here but I think the better thing to do would be a check to be sure metadata_columns is enabled as I think the IS NULL check is very valid here as in normal operation you want activate_version to be keeping your data very "clean" and in the same shape as the source.
I should also write a test for this case as it's potentially scary (only place in our code that we drop tables so being cautious is good)
There was a problem hiding this comment.
If we fix the other issue https://github.com/MeltanoLabs/target-postgres/pull/89/files#r1084618420 I believe this one gets solved as well.
aaronsteers
changed the title
…of ACTIVATE_VERSION sometimes send an ACTIVATE_MESSAGE before records are sent which I think is just a bad tap implemention but it is valid
…gres into activate_version
| | sqlalchemy_url | False | None | SQLAlchemy connection string. This will override using host, user, password, port,dialect. Note that you must esacpe password specialcharacters properly seehttps://docs.sqlalchemy.org/en/20/core/engines.html#escaping-special-characters-such-as-signs-in-passwords | | ||
| | dialect+driver | False | postgresql+psycopg2 | Dialect+driver see https://docs.sqlalchemy.org/en/20/core/engines.html. Generally just leave this alone. Note if sqlalchemy_url is set this will be ignored. | | ||
| | default_target_schema | False | None | Postgres schema to send data to, example: tap-clickup | | ||
| | hard_delete | False | 0 | When activate version is sent from a tap this specefies if we should delete the records that don't match, or mark them with a date in the `_sdc_deleted_at` column. | |
There was a problem hiding this comment.
Agreed. Very strange that it shows defaults of 0. Agreed though we don't need to block on this.
Co-authored-by: Aaron ("AJ") Steers <aj@meltano.com>
Closes #72
Closes #88