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

[Test PR] SNOW-892284: Fix boolean parameter parsing from URL query #446

Merged
merged 11 commits into from
Oct 19, 2023

Conversation

sfc-gh-stan
Copy link
Collaborator

@sfc-gh-stan sfc-gh-stan commented Sep 11, 2023

Duplicate of #439, which was closed accidentally.

cache_column_metadata is a snowflake-sqlalchemy argument, not
a snowflake-connector-python argument so it should be set
and ommitted from the arguments list before the call to the
connector is made.
@saulbein
Copy link
Contributor

Missing a CI run approval at the moment 😞

@saulbein
Copy link
Contributor

Continuing from the thread in #439 (can't post comments in closed PRs): sure, we can merge this PR, though at the moment it requires a review approval 😄

@sfc-gh-stan sfc-gh-stan changed the title Test #439 [Test PR] SNOW-892284: Fix boolean parameter parsing from URL query Oct 12, 2023
@sfc-gh-stan sfc-gh-stan requested review from sfc-gh-aling, a team and sfc-gh-jdu and removed request for a team October 12, 2023 14:39
@sfc-gh-stan
Copy link
Collaborator Author

Requested approvals from @sfc-gh-aling and @snowflakedb/snowpark-python-api.

DESCRIPTION.md Outdated Show resolved Hide resolved
src/snowflake/sqlalchemy/snowdialect.py Show resolved Hide resolved
)

for name, value in query.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I understand what this for-loop is doing, can you elaborate?

Copy link
Contributor

@saulbein saulbein Oct 13, 2023

Choose a reason for hiding this comment

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

We grab the parameter to type mapping used in validation of the driver arguments to figure out what are the expected types of each argument, then we pass the value to the driver according to these rules:

  • if the parameter is not found in the type mapping, pass it through as a string
  • if the expected type is str, pass it through as a string
  • if the expected type is bool, parse it and pass as a boolean
  • if it's some other type, pass it through as a string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification. Let's maybe add it into the comment? :)

@sfc-gh-stan sfc-gh-stan merged commit 8b701a8 into main Oct 19, 2023
@sfc-gh-stan sfc-gh-stan deleted the test_pr_439 branch October 19, 2023 14:34
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants