motherduck integration test improvements#777
Conversation
…robatch error path that left behind temp tabes.
jwills
left a comment
There was a problem hiding this comment.
Reviewed end-to-end and traced the changes against source. The core fix is sound: I verified the Python batch_id derivation in impl.py matches the Jinja chain in incremental.sql:45 exactly (same replacements, same order), and since BatchContext.event_time_start is a datetime and | string is just str(), both sides produce identical identifiers. The getattr(..., "batch", None) fallback safely reproduces the old behavior for non-microbatch incrementals, and the is_motherduck split preserves the original semantics. A few things worth discussing below — the main one is the inactivity-ttl default.
| # (not a local connection with MotherDuck attached), since only | ||
| # instances registered with the instance cache support this setting, | ||
| # and it must be set before SaaS mode locks the config. | ||
| config.setdefault("motherduck_dbinstance_inactivity_ttl", "0s") |
There was a problem hiding this comment.
This sets motherduck_dbinstance_inactivity_ttl=0s on the production adapter path, so it becomes the default for every dbt-duckdb user whose primary connection is MotherDuck — not just CI. That's an aggressive default (instances spin down immediately on inactivity → more cold starts). Is that intended as a global default, or should it be scoped to the test profile (e.g. set via config_options in conftest.py)? The setdefault lets users override, but since the PR title reads as test-only, this out-of-the-box behavior change is worth calling out in the description.
There was a problem hiding this comment.
good catch - this was not intended. but after thinking about it more it is probably the desired behavior. Since the db instance typically lasts a full dbt command/session, the overhead of recreating a duckdb instance for the motherduck connecition (should be < 1s) is still small.
Also, in a regular dbt run invocation, the environment/duckdb instance is only created once - so this setting does not influence the common case behavior. In the case where someone might have a long running process that spanws multiple dbt sessions/commands, the changes (motherduck_dbinstance_inactivity_ttl=0 and cleaning up the env) will also prevent Connection Error: Can't open a connection to same database file with a different configuration than existing connections errors. Overall i think the change is beneficial.
There was a problem hiding this comment.
So that sounds plausible to me, but do we have evidence that it's always better for dbt runs? At the very least we should document the change in the PR so if there is a regression for some use case folks can quickly identify the relevant PR ("integration test improvements" wouldn't jump out to me as as the name of a PR that a default settings change)
|
@hrl20 code looks good (thank you) but the MD test segfaulted somehow? is that a real thing? |
Yeah it looks real 😬 investigating.. |
|
ah it seems to be pyarrow related-- gonna drop back down to 24.0.0 on main |
|
not the first time a pyarrow release has caused something like this |
|
arh hmm-- the "test" db on the testing account is gone, so we may need to drop to 24.0.0 on this PR to get it fixed-- will let you decide #782 |
reverting it in this PR. thanks for finding the issue! really strange.. |
While investigating why MotherDuck integration tests have been taking over 45 minutes, found that one of the microbatch tests left behind many temporary tables that kept accumulating.
This PR fixes the issue on 2 levels: