-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add dbt 1.9 support #46
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
Conversation
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.
👍 Looks good to me! Reviewed everything up to 8a0ece7 in 24 seconds
More details
- Looked at
421lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:116
- Draft comment:
Ensure consistency in accessingresource_type. The change fromnode.resource_type.valuetonode.resource_typeis correct and should be applied consistently across all instances. This change is also applicable in other files likev11/wrapper.pyandv12/wrapper.py. - Reason this comment was not posted:
Comment did not seem useful.
2. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:178
- Draft comment:
Ensuredocsis converted to a dictionary if it exists. The change frommacro.docstomacro.docs.dict()is correct and should be applied consistently across all instances. This change is also applicable in other files likev11/wrapper.pyandv12/wrapper.py. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_uhO3IAMiq8YyCSIX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| contract=contract, | ||
| meta=node.meta, | ||
| patch_path=node.patch_path, | ||
| access=node.access.value, |
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 is in v1.7? Is this because of artifact parser update?
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.
Yes, it's because of that
| schema_name=node.schema_, | ||
| name=node.name, | ||
| resource_type=AltimateResourceType(node.resource_type.value), | ||
| resource_type=AltimateResourceType(node.resource_type), |
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.
is this right?
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 is it mapping between the enums. Please check
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.
node.resource_type is a string now. It is right. I had to do a similar change when we had upgraded to a new version of dbt-artifacts-parser in the past as well, but only for the newest manifest version. Seems like they introduced the same thing for the older manifest versions this time.
setup.py
Outdated
| install_requires=[ | ||
| "click==8.1.7", | ||
| "dbt-artifacts-parser==0.6.0", | ||
| "dbt-artifacts-parser==0.8.1", |
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 you make this
| "dbt-artifacts-parser==0.8.1", | |
| "dbt-artifacts-parser>=0.8.1", |
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.
~=
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.
👍 Looks good to me! Incremental review on 861a571 in 43 seconds
More details
- Looked at
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. setup.py:65
- Draft comment:
Consider verifying if the change from==to~=for version specifiers is compatible with the project's requirements. This change allows for more flexibility but may introduce compatibility issues if not tested thoroughly. - Reason this comment was not posted:
Confidence changes required:50%
The change from '==' to '~=' in the version specifier allows for more flexibility in dependency versions, which is generally a good practice unless specific versions are required. However, this should be verified against the project's compatibility requirements.
Workflow ID: wflow_FPLgl597IcqoGUce
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…bt-1.9-support
mdesmet
left a comment
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.
Please check why tests are falling
|
pydantic.warnings.PydanticDeprecatedSince20: Pydantic warnings are being treated as errors |
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.
👍 Looks good to me! Incremental review on afbaafb in 35 seconds
More details
- Looked at
764lines of code in11files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/insights/checks/check_model_has_labels_keys.py:79
- Draft comment:
The use ofmodel_dump()is correct for Pydantic v1.9+ models. Ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The use ofmodel_dump()is consistent across the codebase for converting Pydantic models to dictionaries. This is a valid change from the previousdict()method, aligning with Pydantic v1.9+ changes.
2. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_freshness.py:64
- Draft comment:
The use ofmodel_dump()is correct for Pydantic v1.9+ models. Ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The use ofmodel_dump()is consistent across the codebase for converting Pydantic models to dictionaries. This is a valid change from the previousdict()method, aligning with Pydantic v1.9+ changes.
3. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:137
- Draft comment:
The use ofmodel_dump()is correct for Pydantic v1.9+ models. Ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The use ofmodel_dump()is consistent across the codebase for converting Pydantic models to dictionaries. This is a valid change from the previousdict()method, aligning with Pydantic v1.9+ changes.
4. src/datapilot/core/platforms/dbt/wrappers/manifest/v11/wrapper.py:137
- Draft comment:
The use ofmodel_dump()is correct for Pydantic v1.9+ models. Ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The use ofmodel_dump()is consistent across the codebase for converting Pydantic models to dictionaries. This is a valid change from the previousdict()method, aligning with Pydantic v1.9+ changes.
5. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:137
- Draft comment:
The use ofmodel_dump()is correct for Pydantic v1.9+ models. Ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The use ofmodel_dump()is consistent across the codebase for converting Pydantic models to dictionaries. This is a valid change from the previousdict()method, aligning with Pydantic v1.9+ changes.
Workflow ID: wflow_vFd2QmfpGk5gwVUp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Tested with tuva manifest and catalog files of dbt versions 1.6, 1.7, 1.8 and 1.9
Important
Add support for dbt 1.9 by updating dependencies, schema handling, and wrapper functions for compatibility.
dbt-artifacts-parserto version 0.8.1 insetup.py.nodesandmacrosinAltimateDependsOninmanifest.py.ConfigDictformodel_configincatalog.pyandmanifest.py._get_node()and_get_macro()inv10/wrapper.py,v11/wrapper.py, andv12/wrapper.pyto handleresource_typedirectly and convertdocsto dict if present.get_tests()andget_seeds()inv11/wrapper.pyandv12/wrapper.pyto handleresource_typewithout.value.NodetoNodes,Node1toNodes1, etc., inv12/schemas.py.This description was created by
for afbaafb. It will automatically update as commits are pushed.