Skip to content
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

Pydantic improvements #901

Merged
merged 36 commits into from
Feb 7, 2024
Merged

Pydantic improvements #901

merged 36 commits into from
Feb 7, 2024

Conversation

sultaniman
Copy link
Contributor

This PR is to address the issue #895 which is when pydantic column schema is provided.
Adjustment recursively calls pydantic_to_table_schema_columns to expand for sub model.

@sultaniman sultaniman requested a review from sh-rp January 19, 2024 10:10
@sultaniman sultaniman self-assigned this Jan 19, 2024
Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit bac3fac
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/65c3996cccafaa00098f9459

Copy link
Collaborator

@codingcyclist codingcyclist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff - I think the behavior between pydantic and other complex constructs is not 100% consistent yet in cases where skip_complex_types = False. I left some comments

dlt/common/libs/pydantic.py Outdated Show resolved Hide resolved
tests/load/pipeline/test_pydantic_models.py Outdated Show resolved Hide resolved
tests/load/pipeline/test_pydantic_models.py Outdated Show resolved Hide resolved
tests/load/pipeline/test_pydantic_models.py Outdated Show resolved Hide resolved
codingcyclist
codingcyclist previously approved these changes Jan 30, 2024
Copy link
Collaborator

@codingcyclist codingcyclist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray, great stuff 🙌

@sultaniman sultaniman marked this pull request as ready for review January 30, 2024 13:05
@sultaniman sultaniman force-pushed the pydantic-improvements branch from 0abf999 to 6556f1b Compare January 30, 2024 13:18
@sultaniman sultaniman requested a review from burnash January 30, 2024 13:36
@sultaniman sultaniman force-pushed the pydantic-improvements branch from 7218345 to 8a58ac0 Compare January 31, 2024 10:40
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we have an edge case when there's pydantic model defined and we have skip_complex_types set to true. please see my review

dlt/common/libs/pydantic.py Outdated Show resolved Hide resolved
dlt/common/libs/pydantic.py Show resolved Hide resolved
dlt/common/libs/pydantic.py Outdated Show resolved Hide resolved
@sultaniman sultaniman force-pushed the pydantic-improvements branch 2 times, most recently from b347fd9 to 6037b04 Compare February 6, 2024 14:49
@sultaniman sultaniman requested a review from rudolfix February 6, 2024 15:28
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sultaniman this looks good now. and I know where the problems with duckdb come from. we have a special area in tests suite for local pipeline tests

tests/pipeline/test_pipeline_extra

you can move your tests there, our workflow will make sure that duckdb and pydantic are installed

@sultaniman sultaniman requested a review from rudolfix February 7, 2024 08:59
@sultaniman sultaniman force-pushed the pydantic-improvements branch from 4d84a02 to ebe6ced Compare February 7, 2024 09:14
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic LGTM!
@sultaniman this code asks for unit tests. we have unit tests for the mapper in libs. why you didn't add the tests there? please test all combinations explictly

tests/pipeline/test_pipeline_extra.py Outdated Show resolved Hide resolved
tests/pipeline/test_pipeline_extra.py Outdated Show resolved Hide resolved
tests/pipeline/test_pipeline_extra.py Outdated Show resolved Hide resolved
tests/pipeline/test_pipeline_extra.py Show resolved Hide resolved
@sultaniman
Copy link
Contributor Author

sultaniman commented Feb 7, 2024

logic LGTM! @sultaniman this code asks for unit tests. we have unit tests for the mapper in libs. why you didn't add the tests there? please test all combinations explictly

@rudolfix do you mean to add tests for TypeMapper in destinations libs/test_pydantic?

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for all the tests!

@rudolfix rudolfix merged commit 754e508 into devel Feb 7, 2024
54 of 57 checks passed
@rudolfix rudolfix deleted the pydantic-improvements branch February 7, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants