-
Notifications
You must be signed in to change notification settings - Fork 173
feat: load driver from uri #3694
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do this, the logic to detect URI/driver from the driver arg needs to be either removed or made the same (as it doesn't split on colon). (Also I think that logic should probably validate that the "scheme" is alphanumeric+a few other symbols instead of accepting anything)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to validate the scheme here rather than just let the driver handle validating the URI itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying to validate the scheme; just avoid a false positive if there's something that looks like a colon slash later in the "uri" but the part before it wouldn't have been a valid scheme in the first place
5a014f8 to
0553228
Compare
|
I have no idea why python is failing to find the bigquery driver shared lib on windows in the CI. Any ideas @lidavidm? |
|
Could you share the CI failure log? |
|
|
@kou turned out that the issue was me badly handling windows file paths. Though I haven't been able to figure out the cause of this failure: https://github.com/apache/arrow-adbc/actions/runs/19449897622/job/55652709570?pr=3694 and the almalinux-9 failure (https://github.com/apache/arrow-adbc/actions/runs/19449897615/job/55652412064?pr=3694) |
|
Great! I'll take a look at them! AlmaLinux 9: Installing old Apache Arrow from EPEL isn't expected. I'll try why it happens on local. |
|
Thanks!! |
|
AlmaLinux 9: I think that https://lists.apache.org/thread/rqsvtdj2jxks03z8wzymk46br71dqd4h is the same problem. Windows: This has been failed since 2964c54 but the change will not be related. Some CI environments may be changed (e.g. package versions in Conda): The first failure log: https://github.com/apache/arrow-adbc/actions/runs/18672884467/job/53237442892 Anyway, this is also not related to this PR. |
Co-authored-by: David Li <[email protected]>
|
@kou Thanks for looking into this, do you think we could create a separate PR to fix the almalinux issue at least? |
|
Yes. But I don't have a correct fix for the AlmaLinux 9 failure for now. (I think that the patch in the mailing list post is a workaround.) So could you create a separated issue for now? |
|
Thanks! |
Allow loading a driver via the driver manager using only a URI