Skip to content
This repository was archived by the owner on Sep 2, 2025. It is now read-only.

Conversation

@peterallenwebb
Copy link
Contributor

@peterallenwebb peterallenwebb commented Mar 1, 2023

resolves #319

Description

Implements RedshiftConnectionManager.data_type_code_to_name() in order to support contract validation.

Checklist

@cla-bot cla-bot bot added the cla:yes label Mar 1, 2023
@peterallenwebb peterallenwebb marked this pull request as ready for review March 1, 2023 19:55
@peterallenwebb peterallenwebb requested a review from a team as a code owner March 1, 2023 19:55
@colin-rogers-dbt
Copy link
Contributor

Looks like this requires a type update:

dbt/adapters/redshift/connections.py:285: error: Argument 1 of "data_type_code_to_name" is incompatible with supertype "SQLConnectionManager"; supertype defines the argument type as "Union[int, str]"  [override]

@peterallenwebb
Copy link
Contributor Author

@colin-rogers-dbt I've fixed it up. It appears the mypy check in CI is more thorough than the local check.

setup.py Outdated
install_requires=[
f"dbt-core~={_core_version()}",
f"dbt-postgres~={_core_version()}",
"dbt-core~=1.5.0a1",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, and we don't intend to merge this PR as-is. All of the changes to dev-requirements.txt will be reverted before merge.

The problem is that the changeset in dbt-core which this PR coordinates with is branched from dbt-core 1.5.0a1, and I don't know if there is a better way to get the dependencies to work out so that tests pass in CI while cross-repo work is in progress. I don't know what final merge strategy is going to be, but as lead on this, @MichelleArk could provide more detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Final merge strategy will be to merge the core changes first, revert any changes made in adapter branches to for testing (e.g. dev-requirements.txt, setup.py changes), run CI against the latest main branch of core on adapter PRs, and merge on green.

return "INTEGER_ARRAY"

@pytest.fixture
def string_array_type(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

this fixture and the int_array_type one shouldn't be necessary anymore! BaseConstraintsColumnsEqual doesn't do anything with them.

Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

one small comment on tests, otherwise LGTM!

@peterallenwebb peterallenwebb merged commit 6d55588 into main Mar 3, 2023
@peterallenwebb peterallenwebb deleted the paw/ct-2103-column-schema-2 branch March 3, 2023 19:45
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
* CT-2103: Implement data_type_code_to_name for redshift

* CT-2103: Hardcode dbt version for testing.

* CT-2103: Update dev-requirements to use branch name

Co-authored-by: Jeremy Cohen <[email protected]>

* CT-2103: Fix mypy complaint

* CT-2103: Provide a Redshift specific override of test cases for new tests

* CT-2103: Remove now-unneeded fixture overrides

* CT-2103: Remove temporary branch hacks in dev-requirements.txt

* CT-2103: Revert temporary branch changes to setup.py

---------

Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: colin-rogers-dbt <[email protected]>
Co-authored-by: Mike Alfare <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CT-2103] Implement get_column_schema_from_query macro

6 participants