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(duckdb): allow connection to motherduck via ibis.connect #8357

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

gforsyth
Copy link
Member

Description of changes

Our _from_url was a little aggressive in calling Path().absolute()
on targets. Since we already handle that in duckdb.do_connect we can
use urlparse to rip out the appropriate bits and forward them.

Issues closed

Fixes #8355

@gforsyth
Copy link
Member Author

Not adding a test because I don't want to monkeypatch a bunch of stuff, but:

 🐍(nix) ~githibis-ibismd_connect⚑16
🐚 ipython
Python 3.10.13 (main, Aug 24 2023, 12:59:26) [GCC 13.2.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.18.1 -- An enhanced Interactive Python. Type '?' for help.

[ins] In [1]: import ibis

[ins] In [2]: con = ibis.connect("duckdb://md:my_db")

[ins] In [3]: con.list_tables()
Out[3]: ['penguins']

Special case if the database path starts with the motherduck prefix
Path(*parts).absolute() if parts and parts != [":memory:"] else ":memory:"
)
database = Path(*parts) if parts and parts != [":memory:"] else ":memory:"
if (strdatabase := str(database)).startswith("md:") or strdatabase.startswith(
Copy link
Member

Choose a reason for hiding this comment

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

Should we set up a secret to allow testing this in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do. I don't think they rotate the tokens particularly often and we've already broken it once. I can put in my credentials -- I don't use motherduck except for testing and I'll confirm that I don't have any payment methods set up in there.

Choose a reason for hiding this comment

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

last I checked you can't rotate your key :) have to request it be done on their end

@lostmygithubaccount
Copy link
Member

tested this and it works for me!

@lostmygithubaccount lostmygithubaccount merged commit 42f45fe into ibis-project:main Feb 15, 2024
74 checks passed
@gforsyth gforsyth deleted the md_connect branch February 15, 2024 20:23
@gforsyth
Copy link
Member Author

gforsyth commented Feb 15, 2024

I can add a test in a followup, but what's the best way to go about that?
do a skip if the expected env-var isn't available?
and then that test will only run on PRs that aren't from forks and then again after merge to main?

@cpcloud
Copy link
Member

cpcloud commented Feb 15, 2024

That seems reasonable to me. This is what we do for Snowflake.

ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
…oject#8357)

## Description of changes

Our `_from_url` was a little aggressive in calling `Path().absolute()`
on targets. Since we already handle that in `duckdb.do_connect` we can
use `urlparse` to rip out the appropriate bits and forward them.


## Issues closed

Fixes ibis-project#8355
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.

bug: connecting to MotherDuck broken on main
3 participants