Add max_table_nesting to resource decorator #1242
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| def test_resource_max_nesting( | ||
| nesting_level: int, expected_num_tables: int, expected_table_names: List[str] | ||
| ): | ||
| @dlt.resource(max_table_nesting=nesting_level) |
There was a problem hiding this comment.
you should have multiple resources with different nesting levels at the same time here, also add a source with a nesting level and check that it is overwritten by the resource setting but kept when there is no resource settingl.
There was a problem hiding this comment.
also test what happens if you load to the same table again with the same pipeline but set no nesting level
There was a problem hiding this comment.
@sh-rp I updated the test with additional check
- setting the
max_table_nesting - then running the pipeline
- unsetting the
max_table_nesting - check if the tables are still there.
sultaniman
force-pushed
the
feat/resource-max-nesting-level
branch
from
0bb86a3
to
d35775a
Compare
| @@ -79,21 +80,27 @@ def _is_complex_type(self, table_name: str, field_name: str, _r_lvl: int) -> boo | |||
| # turn everything at the recursion level into complex type | |||
| max_nesting = self.max_nesting | |||
| schema = self.schema | |||
| table = schema.tables.get(table_name) | |||
| if table: | |||
There was a problem hiding this comment.
are we able to cache nesting per table in extend_table? and here to only access a dict with a cache? why: because this is called for each dict/list in the results so is time critical
There was a problem hiding this comment.
Also I am not sure if I understand extend_table ’s purpose and how to use it, grepping in source code doesn’t give definitive examples I could use, should I just define some dictionary for this? Now that we have getter and setter in a resource we will have to invalidate this as well.
There was a problem hiding this comment.
@rudolfix can you please take a look again?
sultaniman
force-pushed
the
feat/resource-max-nesting-level
branch
from
652446a
to
d5a80ee
Compare
sultaniman
force-pushed
the
feat/resource-max-nesting-level
branch
from
d5a80ee
to
129502e
Compare
There was a problem hiding this comment.
Maybe I'm too picky
- you never read
max_table_nestingonly set it. read it ie. to check if you see the same value as in decorator and if the setter works - max_table_nesting will not see the value coming from the source, right? then also test for it and comment that we do not support it
…urce with max_table_nesting set
… max_table_nesting
This PR addresses #945 and implements
max_table_nestingfor resources