Skip to content

Conversation

@felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Apr 22, 2025

I had to move the initialization of the defaultAppName to the databaseImpl instance to be able to use it when setting options post initialization.

Fixes #2727

@felipecrv felipecrv requested a review from zeroshade as a code owner April 22, 2025 03:43
@github-actions github-actions bot modified the milestone: ADBC Libraries 18 Apr 22, 2025
@felipecrv felipecrv changed the title fix(adbc/driver/snowflake): implement ability to set database options after initialization fix(go/adbc/driver/snowflake): implement ability to set database options after initialization Apr 22, 2025
@felipecrv felipecrv force-pushed the set-option-snowflake branch from 2569445 to 647d42f Compare April 22, 2025 03:49
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

LGTM

Please update the PR description, it seems this does a few other minor things beyond just implementing PostInitOptions

Comment on lines 181 to 182
// cnOptions() is nil is SetOptionInternal() is being called when setting an
// option post-initialization.
Copy link
Member

Choose a reason for hiding this comment

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

Can you reword this? It's not immediately obvious what it's trying to say. (cnOptions is nil if the option is being set post-initialiation. perhaps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the typo... I'm copying your line.

@felipecrv
Copy link
Contributor Author

LGTM

Please update the PR description, it seems this does a few other minor things beyond just implementing PostInitOptions

I guess you're referring to the addition of defaultAppName to the database instance. (?) I added an explanation of why it was required.

@lidavidm lidavidm merged commit b82f532 into apache:main Apr 23, 2025
40 of 41 checks passed
colin-rogers-dbt pushed a commit to dbt-labs/arrow-adbc that referenced this pull request Jun 10, 2025
…ons after initialization (apache#2728)

I had to move the initialization of the `defaultAppName` to the
`databaseImpl` instance to be able to use it when setting options post
initialization.

Fixes apache#2727
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.

go/adbc/driver/snowflake: Unable to set database options post initialization

2 participants