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

fix: Improve read_database check for SQLAlchemy async Session objects #16680

Merged

Conversation

alexander-beedie
Copy link
Collaborator

Closes #16616.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Jun 3, 2024
@alexander-beedie alexander-beedie added the A-io-database Area: reading/writing to databases label Jun 3, 2024
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Good move extracting into a helper function.

It still feels a bit brittle checking for the names of certain types. Perhaps this can be rewritten in the future with a dynamic import of SQLAlchemy and doing proper isinstance checks?

@alexander-beedie
Copy link
Collaborator Author

Perhaps this can be rewritten in the future with a dynamic import of SQLAlchemy and doing proper isinstance checks?

Actually, that's a good move; let's do it now ;)

switch name-based type check for isinstance
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.48%. Comparing base (b0845e3) to head (ce51c5e).
Report is 19 commits behind head on main.

Current head ce51c5e differs from pull request most recent head efb188d

Please upload reports for the commit efb188d to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16680      +/-   ##
==========================================
- Coverage   81.52%   81.48%   -0.04%     
==========================================
  Files        1414     1415       +1     
  Lines      186398   186656     +258     
  Branches     3014     3016       +2     
==========================================
+ Hits       151957   152102     +145     
- Misses      33911    34024     +113     
  Partials      530      530              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexander-beedie alexander-beedie merged commit 68299b1 into pola-rs:main Jun 3, 2024
14 checks passed
@alexander-beedie alexander-beedie deleted the improve-alchemy-async-check branch June 3, 2024 18:41
alexander-beedie added a commit to alexander-beedie/polars that referenced this pull request Jun 3, 2024
Wouittone pushed a commit to Wouittone/polars that referenced this pull request Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io-database Area: reading/writing to databases fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polars read_database with an existing async session object
2 participants