-
Notifications
You must be signed in to change notification settings - Fork 173
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
2003 - PoC: ibis support #2004
base: devel
Are you sure you want to change the base?
2003 - PoC: ibis support #2004
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
dlt/pipeline/pipeline.py
Outdated
@@ -1729,3 +1734,11 @@ def _dataset(self, dataset_type: TDatasetType = "dbapi") -> SupportsReadableData | |||
schema=(self.default_schema if self.default_schema_name else None), | |||
dataset_type=dataset_type, | |||
) | |||
|
|||
def _ibis(self) -> IbisBackend: |
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 don't think our original idea of exposing an ibis dataset via the same methods as the dbapi one makes sense, the interface of ibis is completely different to ours.
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.
but you can use overloads on Literal dataset_type
@overload
def _dataset(self, dataset_type: TDatasetType = "dbapi") -> SupportsReadableDataset:
...
@overload
def _dataset(self, dataset_type: TDatasetType = "ibis") -> IbisBackend:
...
to return different types. hmmm but maybe you are right. we should implement readable dataset interface on ibis.
so maybe we add ibis()
on DBAPI dataset? that will return ibis backend? I do not think adding it on a relation makes sense
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 like the overload, I did not know this was possible...
dlt/destinations/dataset.py
Outdated
raise NotImplementedError() | ||
|
||
ibis = ibis.connect(client.config.credentials.to_native_representation()) | ||
# NOTE: there seems to be no standardized way to set the current dataset / schema in ibis |
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 sure where we should put the stuff that converts our credentials into something IBIS understands. client.config.credentials.to_native_representation()
could work for many cases, but selecting the default dataset will probably be something destination specific..
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.
Also for the filesystem destination this will work quite differently, because we first need to populate the duckdb database and then attach ibis to it.
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.
- part that creates backend should go to dlt/helpers/ibis
- part that converts credentials should go to common/libs/ibis
here we can improve a lot. we already pollute credentials in common with concrete converting functions (like fsspec, delta etc.) we should create some universal mechanism to convert them.
2adb193
to
5befef9
Compare
@@ -165,13 +165,18 @@ def open_connection(self) -> duckdb.DuckDBPyConnection: | |||
# set up dataset | |||
if not self.has_dataset(): | |||
self.create_dataset() | |||
print("CREATE") |
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.
hmmm
dlt/pipeline/pipeline.py
Outdated
@@ -1729,3 +1734,11 @@ def _dataset(self, dataset_type: TDatasetType = "dbapi") -> SupportsReadableData | |||
schema=(self.default_schema if self.default_schema_name else None), | |||
dataset_type=dataset_type, | |||
) | |||
|
|||
def _ibis(self) -> IbisBackend: |
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.
but you can use overloads on Literal dataset_type
@overload
def _dataset(self, dataset_type: TDatasetType = "dbapi") -> SupportsReadableDataset:
...
@overload
def _dataset(self, dataset_type: TDatasetType = "ibis") -> IbisBackend:
...
to return different types. hmmm but maybe you are right. we should implement readable dataset interface on ibis.
so maybe we add ibis()
on DBAPI dataset? that will return ibis backend? I do not think adding it on a relation makes sense
dlt/common/destination/reference.py
Outdated
"""fetch arrow table of first 'chunk_size' items""" | ||
... | ||
|
||
def ibis(self, chunk_size: int = None) -> Optional[IbisTable]: ... |
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'd add it on ReadableDBAPIDataset
and return implementation of it in _dataset
of pipeline
dlt/destinations/dataset.py
Outdated
@@ -42,18 +90,91 @@ def __init__( | |||
self.iter_arrow = self._wrap_iter("iter_arrow") # type: ignore | |||
self.iter_fetch = self._wrap_iter("iter_fetch") # type: ignore | |||
|
|||
# TODO: where should this go, should cursors support "native" ibis or do we do a conversion somewhere | |||
def ibis(self, chunk_size: int = None) -> Optional[IbisTable]: |
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.
this does not make sense... we have stuff materialized. way better to just do df() which has data frame interface
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 removed those
dlt/common/destination/reference.py
Outdated
"""iterate over arrow tables of 'chunk_size' items""" | ||
... | ||
|
||
def iter_ibis(self, chunk_size: int) -> Generator[IbisTable, None, None]: ... |
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.
how are you going to implement that? you'll need to create IbisTable on a chunk of data which is lazy evaluated. otherwise does not make sense
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.
removed it
dlt/destinations/dataset.py
Outdated
raise NotImplementedError() | ||
|
||
ibis = ibis.connect(client.config.credentials.to_native_representation()) | ||
# NOTE: there seems to be no standardized way to set the current dataset / schema in ibis |
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.
- part that creates backend should go to dlt/helpers/ibis
- part that converts credentials should go to common/libs/ibis
here we can improve a lot. we already pollute credentials in common with concrete converting functions (like fsspec, delta etc.) we should create some universal mechanism to convert them.
dlt/destinations/dataset.py
Outdated
|
||
def head(self) -> "ReadableDBAPIRelation": | ||
return self.limit(5) | ||
|
||
|
||
class ReadableDBAPIDataset(SupportsReadableDataset): |
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.
let's move ibis() here. then we hand over the connection and return the ibis backend
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.
can we move schema property to the Protocol?
dlt/destinations/dataset.py
Outdated
raise NotImplementedError() | ||
|
||
# NOTE: there seems to be no standardized way to set the current dataset / schema in ibis | ||
ibis.raw_sql(f"SET search_path TO {dataset_name};") |
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.
why? you just do:
return ibis[dataset_name]
to select namespace.
https://duckdb.org/docs/guides/python/ibis.html
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.
where does it say that on this page? In any case it does not work and I also looked for a bit before and could not find any standard way to select the current database (as it is called in ibis). The call the whole thing (in the case of duckdb the file) a catalog and what we call dataset is called database there. You can list the database but not set a default one.. At least I have not found a method.
also here you wrote a helper: https://github.com/dlt-hub/dlt/pull/1491/files |
59b0977
to
ab62c59
Compare
ab62c59
to
500b5ff
Compare
Description
First draft of ibis support to figure out how we would like to implement this. Please also read the attached ticket for the considered approaches.
Ibis tables should work for all destinations, full ibis connection now is implemented for postgres, filesystem and duckdb in this PoC
Discussion at ibis about using ibis only as a query builder: ibis-project/ibis#10452