Add ability to read-write to SQL databases.
NONE
_The documentation is not available anymore as the PR was closed or merged._
2022-09-03T19:15:47Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1236184622
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5Jrq4u
https://api.github.com/repos/huggingface/datasets/issues/comments/1236184622/reactions
2022-10-03T16:34:36Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1236184622
https://avatars.githubusercontent.com/u/99929124?v=4
https://api.github.com/users/HuggingFaceDocBuilderDev/events{/privacy}
https://api.github.com/users/HuggingFaceDocBuilderDev/followers
https://api.github.com/users/HuggingFaceDocBuilderDev/following{/other_user}
https://api.github.com/users/HuggingFaceDocBuilderDev/gists{/gist_id}
https://github.com/HuggingFaceDocBuilderDev
HuggingFaceDocBuilderDev
U_kgDOBfTMJA
https://api.github.com/users/HuggingFaceDocBuilderDev/orgs
https://api.github.com/users/HuggingFaceDocBuilderDev/received_events
https://api.github.com/users/HuggingFaceDocBuilderDev/repos
https://api.github.com/users/HuggingFaceDocBuilderDev/starred{/owner}{/repo}
https://api.github.com/users/HuggingFaceDocBuilderDev/subscriptions
User
https://api.github.com/users/HuggingFaceDocBuilderDev
public
CONTRIBUTOR
Ah CI runs with `pandas=1.3.5` which doesn't return the number of row inserted.
2022-09-03T20:41:27Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1236195624
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5Jrtko
https://api.github.com/repos/huggingface/datasets/issues/comments/1236195624/reactions
2022-09-03T20:41:27Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1236195624
https://avatars.githubusercontent.com/u/8976546?v=4
https://api.github.com/users/Dref360/events{/privacy}
https://api.github.com/users/Dref360/followers
https://api.github.com/users/Dref360/following{/other_user}
https://api.github.com/users/Dref360/gists{/gist_id}
https://github.com/Dref360
Dref360
MDQ6VXNlcjg5NzY1NDY=
https://api.github.com/users/Dref360/orgs
https://api.github.com/users/Dref360/received_events
https://api.github.com/users/Dref360/repos
https://api.github.com/users/Dref360/starred{/owner}{/repo}
https://api.github.com/users/Dref360/subscriptions
User
https://api.github.com/users/Dref360
public
MEMBER
wow this is super cool!
2022-09-05T10:16:47Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1236811159
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5JuD2X
https://api.github.com/repos/huggingface/datasets/issues/comments/1236811159/reactions
2022-09-05T10:16:47Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1236811159
https://avatars.githubusercontent.com/u/326577?v=4
https://api.github.com/users/julien-c/events{/privacy}
https://api.github.com/users/julien-c/followers
https://api.github.com/users/julien-c/following{/other_user}
https://api.github.com/users/julien-c/gists{/gist_id}
https://github.com/julien-c
julien-c
MDQ6VXNlcjMyNjU3Nw==
https://api.github.com/users/julien-c/orgs
https://api.github.com/users/julien-c/received_events
https://api.github.com/users/julien-c/repos
https://api.github.com/users/julien-c/starred{/owner}{/repo}
https://api.github.com/users/julien-c/subscriptions
User
https://api.github.com/users/julien-c
public
CONTRIBUTOR
@lhoestq I'm getting error in integration tests, not sure if it's related to my PR. Any help would be appreciated :) 

```
if not self._is_valid_token(token):
>               raise ValueError("Invalid token passed!")
E               ValueError: Invalid token passed!
```
2022-09-06T14:28:43Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1238231731
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5Jzeqz
https://api.github.com/repos/huggingface/datasets/issues/comments/1238231731/reactions
2022-09-06T14:28:43Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1238231731
https://avatars.githubusercontent.com/u/8976546?v=4
https://api.github.com/users/Dref360/events{/privacy}
https://api.github.com/users/Dref360/followers
https://api.github.com/users/Dref360/following{/other_user}
https://api.github.com/users/Dref360/gists{/gist_id}
https://github.com/Dref360
Dref360
MDQ6VXNlcjg5NzY1NDY=
https://api.github.com/users/Dref360/orgs
https://api.github.com/users/Dref360/received_events
https://api.github.com/users/Dref360/repos
https://api.github.com/users/Dref360/starred{/owner}{/repo}
https://api.github.com/users/Dref360/subscriptions
User
https://api.github.com/users/Dref360
public
MEMBER
I just relaunched the tests, it should be fixed now
2022-09-06T14:35:53Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1238241390
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5JzhBu
https://api.github.com/repos/huggingface/datasets/issues/comments/1238241390/reactions
2022-09-06T14:35:53Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1238241390
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
COLLABORATOR
Thanks a lot for working on this!

I have some concerns with the current design:
* Besides SQLite, the loader should also work with the other engines supported by SQLAlchemy. (A better name for it in the current state would be `sqlite` :))
* It should support arbitrary queries/table names - only the latter currently works.
* Exposing this loader as a packaged builder (`load_dataset("sql", ...)`) is not a good idea for the following reasons:
    * Considering the scenario where a table with the same name is present in multiple files is very unlikely, the data files resolution is not needed here. And if we remove that, what the name of the default split should be? "train"?
    * `load_dataset("sql", ...)` also implies that streaming should work, but that's not the case.  And I don't think we can change that, considering how hard it is to make SQLite files streamable.

All this makes me think we shouldn't expose this builder as a packaged module and, instead, limit the API to `Dataset.from_sql`/`Dataset.to_sql` (with the signatures matching the ones in pandas as much as possible; regarding this, note that SQLAlchemy connections are not hashable/picklable, which is required for caching, but I think it's OK only to allow URI strings as connections to bypass that (Dask has the same limitation).

WDYT?
2022-09-07T18:50:50Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1239757927
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5J5TRn
https://api.github.com/repos/huggingface/datasets/issues/comments/1239757927/reactions
2022-09-07T18:53:42Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1239757927
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
CONTRIBUTOR
Hi @mariosasko thank you for your review.

I agree that `load_dataset('sql',...)` is a bit weird and I would be happy to remove it. To be honest, I only added it when I saw that it was the preferred way in `loading.mdx`. 

I agree that the `SELECT` should be a parameters as well. I'll add it.

So far, only `Dataset.to_sql` explicitly supports any SQLAlchemy Connexion, I'm pretty sure that `Dataset.from_sql` would work with a Connexion as well, but it would break the typing from the parent class which is `path_or_paths: NestedDataStructureLike[PathLike]`. I would prefer not to break this API Contract.


I will have time to work on this over the weekend. Please let me know what you think if I do the following:
* Remove `load_dataset('sql', ...)` and edit the documentation to use `to_sql, from_sql`.
* Tentatively make `Dataset.from_sql` typing work with SQLAlchemy Connexion.
* Add support for custom queries (Default would be `SELECT * FROM {table_name}`).

Cheers!
2022-09-08T01:00:55Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1240078415
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5J6hhP
https://api.github.com/repos/huggingface/datasets/issues/comments/1240078415/reactions
2022-09-08T01:00:55Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1240078415
https://avatars.githubusercontent.com/u/8976546?v=4
https://api.github.com/users/Dref360/events{/privacy}
https://api.github.com/users/Dref360/followers
https://api.github.com/users/Dref360/following{/other_user}
https://api.github.com/users/Dref360/gists{/gist_id}
https://github.com/Dref360
Dref360
MDQ6VXNlcjg5NzY1NDY=
https://api.github.com/users/Dref360/orgs
https://api.github.com/users/Dref360/received_events
https://api.github.com/users/Dref360/repos
https://api.github.com/users/Dref360/starred{/owner}{/repo}
https://api.github.com/users/Dref360/subscriptions
User
https://api.github.com/users/Dref360
public
COLLABORATOR
Perhaps after we merge https://github.com/huggingface/datasets/pull/4957 (**Done!**), you can subclass `AbstractDatasetInputStream` instead of `AbstractDatasetReader` to not break the contract with the connection object. Also, let's avoid having the default value for the query/table (you can set it to `None` in the builder and raise an error in the builder config's `__post_init__` if it's not provided). Other than that, sounds good!
2022-09-08T17:55:46Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1241047176
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5J-OCI
https://api.github.com/repos/huggingface/datasets/issues/comments/1241047176/reactions
2022-09-16T15:30:10Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1241047176
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
COLLABORATOR
@Dref360 I've made final changes/refinements to align the SQL API with Pandas/Dask. Let me know what you think.

2022-09-21T15:57:28Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1253908314
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5KvR9a
https://api.github.com/repos/huggingface/datasets/issues/comments/1253908314/reactions
2022-09-21T15:57:28Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1253908314
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
CONTRIBUTOR
Thank you so much! I was missing a lot of things sorry about that.
LGTM
2022-09-21T17:28:54Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1254013185
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5KvrkB
https://api.github.com/repos/huggingface/datasets/issues/comments/1254013185/reactions
2022-09-21T17:28:54Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1254013185
https://avatars.githubusercontent.com/u/8976546?v=4
https://api.github.com/users/Dref360/events{/privacy}
https://api.github.com/users/Dref360/followers
https://api.github.com/users/Dref360/following{/other_user}
https://api.github.com/users/Dref360/gists{/gist_id}
https://github.com/Dref360
Dref360
MDQ6VXNlcjg5NzY1NDY=
https://api.github.com/users/Dref360/orgs
https://api.github.com/users/Dref360/received_events
https://api.github.com/users/Dref360/repos
https://api.github.com/users/Dref360/starred{/owner}{/repo}
https://api.github.com/users/Dref360/subscriptions
User
https://api.github.com/users/Dref360
public
COLLABORATOR
I think we can merge if the tests pass. 

One last thing I would like to get your opinion on - currently, if SQLAlchemy is not installed, the missing dependency error will be thrown inside `pandas.read_sql`. Do you think we should be the ones throwing this error, e.g. after the imports in `packaged_modules/sql/sql.py` if `SQLALCHEMY_AVAILABLE` is `False` (note that this would mean making `sqlalchemy` a required dependency for the docs to be able to add `SqlConfig` to the package reference)?
2022-09-23T13:54:53Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1256248408
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5K4NRY
https://api.github.com/repos/huggingface/datasets/issues/comments/1256248408/reactions
2022-09-23T13:54:53Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1256248408
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
MEMBER
> One last thing I would like to get your opinion on - currently, if SQLAlchemy is not installed, the missing dependency error will be thrown inside pandas.read_sql

Is sqlalchemy always required for pd.read_sql ? If so, I think we can raise the error on our side.
But sqlalchemy should still be an optional dependency for `datasets` IMO
2022-09-23T14:48:10Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1256312260
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5K4c3E
https://api.github.com/repos/huggingface/datasets/issues/comments/1256312260/reactions
2022-09-23T14:48:10Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1256312260
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
COLLABORATOR
@lhoestq 
> Is sqlalchemy always required for pd.read_sql ? If so, I think we can raise the error on our side.

In our case, it's always required as we only support database URIs.

> But sqlalchemy should still be an optional dependency for datasets IMO

Yes, it will remain optional for datasets but will be required for building the docs (as is`s3fs`, for instance). 
2022-09-23T14:59:28Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1256327074
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5K4gei
https://api.github.com/repos/huggingface/datasets/issues/comments/1256327074/reactions
2022-09-23T14:59:28Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1256327074
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
MEMBER
Ok I see ! Sounds good :)
2022-09-26T09:35:58Z
https://github.com/huggingface/datasets/pull/4928#issuecomment-1257763587
https://api.github.com/repos/huggingface/datasets/issues/4928
IC_kwDODunzps5K9_MD
https://api.github.com/repos/huggingface/datasets/issues/comments/1257763587/reactions
2022-09-26T09:35:58Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1257763587
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
Fixes #3094 

Add ability to read/write to SQLite files and also read from any SQL database supported by SQLAlchemy.

I didn't add SQLAlchemy as a dependence as it is fairly big and it remains optional. 

I also recorded a Loom to showcase the feature.

https://www.loom.com/share/f0e602c2de8a46f58bca4b43333d541f