Skip to content

Fix(duckdb): Only SET connector_config values on cursor init if they are different#4981

Merged
erindru merged 2 commits intomainfrom
erin/duckdb-cursor-init-idempotency
Jul 17, 2025
Merged

Fix(duckdb): Only SET connector_config values on cursor init if they are different#4981
erindru merged 2 commits intomainfrom
erin/duckdb-cursor-init-idempotency

Conversation

@erindru
Copy link
Collaborator

@erindru erindru commented Jul 17, 2025

Prior to this, a duckdb connection config like:

type: duckdb
connector_config:
  temp_directory: /tmp/duckdb/
  memory_limit: '1GB'

would result in a SET call for each item in connector_config every time a new cursor was initialized.

This isnt a problem for many properties, but ones like temp_directory have side effects. If DuckDB has had to spill data to disk (and created files in temp_directory), if you try to SET temp_directory='...', even to the same value, it will throw an error:

duckdb.duckdb.NotImplementedException: Not implemented Error: Cannot switch temporary directory after the current one has been used

This PR changes the cursor init to read the existing values and only make SET calls if the configured values differ from what is already set

@erindru erindru force-pushed the erin/duckdb-cursor-init-idempotency branch from c0dd637 to 1bf06cf Compare July 17, 2025 03:39
option_names,
)

existing_values = {r[0]: r[1] for r in cursor.fetchall()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
existing_values = {r[0]: r[1] for r in cursor.fetchall()}
existing_values = {field: setting for field, setting in cursor.fetchall()}

nit: maybe something more explicit like this so that it's more clear when going through the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, it didnt even occur to me to unpack the tuple like that. will update

raise ConfigError(f"Failed to set connector config {field} to {setting}: {e}")
if self.connector_config:
option_names = list(self.connector_config)
in_part = ",".join(["?" for _ in range(len(option_names))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in_part = ",".join(["?" for _ in range(len(option_names))])
in_part = ",".join("?" for _ in option_names)

nit: this should achieve the same

@erindru erindru merged commit 2b702ba into main Jul 17, 2025
27 checks passed
@erindru erindru deleted the erin/duckdb-cursor-init-idempotency branch July 17, 2025 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants