prevent accidental wrapping of sources in resources when using adapters #1645
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
dlt/destinations/utils.py
Outdated
| # prevent accidentally wrapping sources with adapters | ||
| if isinstance(data, DltSource): | ||
| raise Exception( | ||
| "You are trying to use an adapter on a dlt source. You can only use adapters on pure" |
There was a problem hiding this comment.
Is this function only used with adapters? The error message seems to be out of context for ensure_resource function. Also, TypeError maybe?
There was a problem hiding this comment.
Yes, it only is used in adapters, I checked that.
dlt/destinations/utils.py
Outdated
| "You are trying to use an adapter on a dlt source. You can only use adapters on pure" | ||
| " data or dlt resources." | ||
| ) | ||
| resource_name = None if hasattr(data, "__name__") else "content" |
There was a problem hiding this comment.
I think this else close should throw a warning. Otherwise, you get some unrecognizable table in the destination with the name Content out of nowhere 😱
rudolfix
left a comment
rudolfix
left a comment
There was a problem hiding this comment.
LGTM! but let's adapt sources with single resource as well
| if isinstance(data, DltResource): | ||
| return data | ||
| # prevent accidentally wrapping sources with adapters | ||
| if isinstance(data, DltSource): |
There was a problem hiding this comment.
could you check if source selects just one resource and then adapt it? look here: #1644 I think the user's thinking is OK
|
@VioletM we should document how to adapt sources. the example below IMO is not the best pattern: beacuse it takes the resource out of source and adapts it and loads it. what user should do: |
add support for source with single resource add tests
|
I have added support for sources with one resource as well as some tests. I'm not sure we should support setting of this default "content" resource name. I have a feeling we should raise similar to when loading data without table name.. |
dlt/destinations/utils.py
Outdated
| resource_name = None if hasattr(data, "__name__") else "content" | ||
| # prevent accidentally wrapping sources with adapters | ||
| if isinstance(data, DltSource): | ||
| if len(data.resources.keys()) == 1: |
There was a problem hiding this comment.
please use "selected_resources". only those are evaluated
dlt/destinations/utils.py
Outdated
|
|
||
| resource_name = None | ||
| if not hasattr(data, "__name__"): | ||
| logger.warning("Setting default resource name to `content` for adapted resource.") |
There was a problem hiding this comment.
this is not warning. IMO passing "dumb" data like a list of dicts is pretty normal. so maybe info level?
Description
A couple of times our users used a destination adapter on a source which creates unexpected results. The small change in this PR changes this.