bug: SQL `prepare_table` runs on setup in stead of drain

[kgpayne]

Singer SDK Version

0.48.0

Is this a regression?

• Yes

Python Version

3.12

Bug scope

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

Operating System

MacOS

Description

I think this might be a regression, but I'm not 100% sure. Current behavior:

• Schema message recieved, SQLSink setup() called on Sink instantiation.
• .setup() calls self.connector.prepare_table() which creates or updates the target schema. In the case of TargetLoadMethods.OVERWRITE (new target default 😬) the destination table is dropped and recreated.
• If a new schema message is received, the existing Sink instance is retired to _sinks_to_clear and a new Sink is instantiated. This new Sink has .setup() called which triggers .setup() evolves or recreates the table using the new schema 😱 Records are held in _sinks_to_clear waiting for a drain_all event.
• When drain_all is eventually called, sinks drain in order, starting with _sinks_to_clear first. However the target schema has now changed (possibly multiple times), and previously received records no longer conform. In the case of the test checking for multiple schema messages any messages inserted by a drain_one trigger will be erased when a new schema message arrives, leading to an incomplete sync 🐛

If I am not mistaken, this means SQL targets currently don't support multiple schemas in a single stream safely 🚨

Link to Slack/Linen

No response

[edgarrmondragon]

Hey Ken, nice to see you around👋

This sounds like a serious bug. I'll investigate and release a fix ASAP if this is indeed dropping data 😬

[kgpayne]

Thanks @edgarrmondragon 🚀 Nice to hear from you, and nice to be back in the SDK codebase. Really exciting to see the progress made in ~1 year since I last checked in 🙌 We are starting to use Meltano a bit with Databricks at Gousto, so I've been building a target using the Databricks SQLAlchemy dialect. Fun to be building Singer plugins again 🙂 Might be raising a few more PR's in the near future if our POC goes well 🤞

[kgpayne]

Thinking about this more, I'm not sure why we queue sink instances for draining at all 🤔 If a new schema message arrives, wouldn't we always want to drain the old messages first before instantiating a new Sink? Struggling to remember why the _sinks_to_clear behaviour was implemented in the first place 😅 Apart from anything else, it's less memory efficient too having records hanging around waiting to be drained.

Will take a snoop of the git history on Monday to see if anything surfaces 🙂

[kgpayne]

The _sinks_to_clear behavior is from the initial Target implementation, and I can't find anything enlightening as to why they are queued rather than drained 🤔

[kgpayne]

@edgarrmondragon did you have a chance to check this out? Anything I can do to help?

[edgarrmondragon]

@edgarrmondragon did you have a chance to check this out? Anything I can do to help?

@kgpayne Does #3340 seem correct to you?

[kgpayne]

@edgarrmondragon left a comment on #3340 🙂 I don't think the issue is resolved, but happy to discuss in Office Hours.