bug: activate_version deletes rows inserted in earlier batches (and deletes despite hard_delete not being set)

[msg555]

Singer SDK Version

0.30.0

Is this a regression?

• Yes

Python Version

3.9

Bug scope

Targets (data type handling, batching, SQL object generation, etc.)

Operating System

Linux

Description

I was attempting to transition to using meltanolabs-target-snowflake (version 0.5.1) from the pipelinewise variant.

When I run EL from a tap_mysql (pipelinewise-tap-mysql) to this target on a table that has 21k rows, I find that after EL completes the destination table only has 1k rows instead. If I turn off hard_delete I instead end up with the full 21k rows. From some investigation it appears that this code snippet is the problem:

sdk/singer_sdk/sinks/sql.py

Lines 381 to 389 in 299acc0

	if self.config.get("hard_delete", True):
	with self.connector._connect() as conn, conn.begin(): # noqa: SLF001
	conn.execute(
	sqlalchemy.text(
	f"DELETE FROM {self.full_table_name} " # noqa: S608
	f"WHERE {self.version_column_name} <= {new_version}",
	),
	)
	return

I see two problems here:

• This seems to be defaulting to hard_delete being enabled, which is not the documented default
• This is deleting rows with the current version, should this be < instead of <=? That would seem to agree with the soft delete logic below this.

Proposed patch might look like

        if self.config.get("hard_delete", False):
            with self.connector._connect() as conn, conn.begin():  # noqa: SLF001
                conn.execute(
                    sqlalchemy.text(
                        f"DELETE FROM {self.full_table_name} "  # noqa: S608
                        f"WHERE {self.version_column_name} < {new_version}",
                    ),
                )
            return

Loader config

    - name: target-snowflake
      variant: meltano
      config:
        account: ${TARGET_SNOWFLAKE_ACCOUNT}
        user: ${TARGET_SNOWFLAKE_USER}
        password: ${TARGET_SNOWFLAKE_PASSWORD}
        database: MY_DATABASE
        default_target_schema: MY_SCEHMA
        warehouse: MY_WAREHOUSE
        add_record_metadata: true

Code

No response

[msg555]

One thing I was unsure of is if it's expected for activate_version to be called multiple times during EL. Is there a chance the tap is the one misbehaving? (although the default still would be wrong in that case)

[edgarrmondragon]

Hi @msg555!

Thanks for logging and for the detail investigation into the issue. This does seem like a significant problem that could lead to unexpected data loss.

This is deleting rows with the current version, should this be < instead of <=? That would seem to agree with the soft delete logic below this.

Yeah, I think that's right.

This seems to be defaulting to hard_delete being enabled, which is not the documented default

Trying to think what users would experience by changing the default to not hard delete. Would they just start seeing that data is now upserted/soft-deleted instead of removed and would that cause issues for downstream data modeling? I still think we should apply your suggested patch but we should call out this change in the release notes of both the SDK and downstream targets.

One thing I was unsure of is if it's expected for activate_version to be called multiple times during EL. Is there a chance the tap is the one misbehaving? (although the default still would be wrong in that case)

I think there would be at most one activate_version message for each stream in the tap, but I could be wrong. What's the tap in question?

cc @pnadolny13 Curious if you've seen this problem come up with target-snowflake. And fwiw target-postgres seems to do the right thing.

[msg555]

Ah, apologies if I've identified the wrong repository. Perhaps the code you linked that's directly in target-snowflake is what is relevant in my case; it still seems to use the <= rather than < operator. Perhaps I should create a new issue over there.

I think there would be at most one activate_version message for each stream in the tap, but I could be wrong. What's the tap in question?

The tap is pipelinewise-tap-mysql==1.5.6

    - name: tap-mysql
      variant: transferwise
      pip_url: pipelinewise-tap-mysql~=1.5.0
      config:
        host: ${TAP_MYSQL_HOST}
        port: ${TAP_MYSQL_PORT}
        user: ${TAP_MYSQL_USER}
        password: ${TAP_MYSQL_PASSWORD}
        filter_dbs: my_db
        session_sqls:
          - SET @@session.max_statement_time=0
          - SET @@session.net_read_timeout=3600
          - SET @@session.net_write_timeout=3600
          - SET @@session.wait_timeout=28800
          - SET @@session.innodb_lock_wait_timeout=3600
      select:
         ...

[edgarrmondragon]

Ah, apologies if I've identified the wrong repository. Perhaps the code you linked that's directly in target-snowflake is what is relevant in my case; it still seems to use the <= rather than < operator. Perhaps I should create a new issue over there.

The activate_version method is not re-implemented by target-snowflake so this still seems like the right place to look at the issue, imo.

The tap is pipelinewise-tap-mysql==1.5.6

Thanks! The tap does seem to emit a single message per stream: https://github.com/transferwise/pipelinewise-tap-mysql/blob/572e08a3576702895e2a9edae188773ec9d7a096/tap_mysql/sync_strategies/full_table.py#L137-L138

[edgarrmondragon]

On a second thought, an issue and PR are probably needed for target-snowflake since failing tests are blocking bumping the SDK: MeltanoLabs/target-snowflake#105

[pnadolny13]

@edgarrmondragon I havent noticed to me honest. I dont use activate version for anything really.

I agree though that seeing hard_delete defaulting to true seems weird. I also dont think hard_delete is registered as a default config because the hub/--about isnt listing it.

In terms of breaking changes I also agree that its better to break someone by getting them back to the behavior they expect vs leaving the bug.