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.

Sign up for GitHub

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

Jump to bottom

Allow override of Column string and numeric type by classes inheritin… #4604

Merged
merged 3 commits into from
Feb 14, 2022
Merged

Allow override of Column string and numeric type by classes inheritin… #4604

merged 3 commits into from
Feb 14, 2022
+4 −2

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Jan 20, 2022 •
edited by McKnight-42
Loading

…g from the Column class

resolves #4603

Description

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Jan 20, 2022
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label Jan 21, 2022
@mdesmet mdesmet mentioned this pull request Jan 24, 2022
Closed
@mdesmet mdesmet requested review from a team as code owners January 31, 2022 10:55
@mdesmet mdesmet requested review from gshank and iknox-fa and removed request for a team January 31, 2022 10:55
@mdesmet
Copy link
Contributor Author

mdesmet commented Feb 1, 2022 •
edited
Loading

@gshank, @iknox-fa, @jtcohen6,

This is pretty important change for dbt-trino and potential other adapters to pickup the inherited methods instead of the core methods. can you have a look at this please?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 3, 2022

@mdesmet Sorry for the delay! We're working on getting more folks up to speed, to help with prompt review for external contributions going forward. I'm really appreciative of your patience in the meantime.

@VersusFacit @McKnight-42 Let's aim to tackle the triage of #4604 and review of this PR in the current sprint. As explained in Slack, we should target v1.1 for this change, since it represents a change to the adapter interface, and it isn't resolving a regression / net-new bug from v1.0.

@McKnight-42 McKnight-42 self-requested a review February 3, 2022 14:50
@leahwicz leahwicz closed this Feb 3, 2022
@leahwicz leahwicz reopened this Feb 3, 2022
@leahwicz
Copy link
Contributor

leahwicz commented Feb 3, 2022

Clsoing and reopening to get GitHub Checks to trigger.

@gshank gshank removed their request for review February 3, 2022 18:02
@McKnight-42
Copy link
Contributor

@mdesmet it seems we have one test failing for for flake8

the error is : fix a indention issue as noted by core/dbt/adapters/base/column.py:45:40: E127 continuation line over-indented for visual indent

cause: seems like the name change from column to self is just making the indention a tad off.

suggested solution: A backspace or two should cause the flake8 tests to pass on Ln 45

you should be able to double check this locally by running tox if you have any questions please reach out.

@McKnight-42
Copy link
Contributor

@mdesmet sorry for the slow feedback, looks like we have tests passing which is great but now your branch is behind main by a commit or two, would you mind updating your local and repushing up so we can keep both in line with each other?

@mdesmet
Copy link
Contributor Author

mdesmet commented Feb 11, 2022

@McKnight-42 : Done!

McKnight-42
McKnight-42 approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

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

Looks like a smart change to be able to overwrite types LGTM

@McKnight-42
Copy link
Contributor

@mdesmet just a heads up a recent change in the black formatter we added caused a test to fail hopped on to make sure it would work. right before merging, thank you so much for working on this.

@McKnight-42 McKnight-42 merged commit a642b20 into dbt-labs:main Feb 14, 2022
timle2 pushed a commit to timle2/dbt-core that referenced this pull request Feb 21, 2022
@mdesmet @McKnight-42 @sp-tim
Allow override of Column string and numeric type by classes inheritin… (
7a29ab8 
dbt-labs#4604)

* Allow override of Column string and numeric type by classes inheriting from the Column class

* updating based on new black formatter

Co-authored-by: Matthew McKnight <[email protected]>
Co-authored-by: Matthew McKnight <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@McKnight-42 McKnight-42 McKnight-42 approved these changes

@iknox-fa iknox-fa Awaiting requested review from iknox-fa iknox-fa was automatically assigned from dbt-labs/core-group

Assignees
No one assigned
Labels
cla:yes Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-78] [Bug] Adapters should be able to override string and numeric type of columns
4 participants
@mdesmet @jtcohen6 @leahwicz @McKnight-42