Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support setting session properties on a individual statement #339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Feb 28, 2023

Description

Sometimes people want to set certain session properties on a single statement. This feature makes this much easier to manage as it avoids the state management involved with SET to the desired value and resetting back to the original value (which might not be the default value so RESET cannot be used).

Other DBMS have similar feature, eg. statement_params in Snowflake.

Some common use cases are setting different limits on query time or INSERT OVERWRITE in hive.

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(*) Release notes are required, with the following suggested text:

* Support setting session properties on individual statements

@cla-bot cla-bot bot added the cla-signed label Feb 28, 2023
@mdesmet mdesmet self-assigned this Feb 28, 2023
@hashhar
Copy link
Member

hashhar commented Mar 3, 2023

@ebyhr opinions? I'm not in favour of this since it makes it harder to reason about how session properties at connection and cursor level interact and what happens with cursor reuse.

Note that most other db-api clients follow the creating separate Connection per unique set of session properties mechanism.

@ebyhr
Copy link
Member

ebyhr commented Mar 8, 2023

@hashhar I have the same opinion as you wrote. No strong opinion though.

@mdesmet
Copy link
Contributor Author

mdesmet commented Mar 8, 2023

I'm not in favour of this since it makes it harder to reason about how session properties at connection and cursor level interact and what happens with cursor reuse.

The counter argument is that this makes setting session properties for individual queries much easier for the end user. It also pretty transparent as the property is only valid for that cursor instance.

I don't think creating separate connections makes sense, I have the opposing view this actually adds complexity for the user. Also what if at some point we introduce transactions (Iceberg). It still makes sense to apply separate session properties onto a certain query within the same connection in that case. This is obviously already possible with SET and RESET session property. The advantage of this method is that the user doesn't have to keep track of the session property state (which is not necessarily the default when using RESET).

@mdesmet mdesmet force-pushed the feat/statement_properties branch from acd2a24 to de7d26e Compare April 27, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants