10000 Fix the table name when reflect is True in sqla.CopyToTable (#2604) by garaud · Pull Request #2605 · spotify/luigi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix the table name when reflect is True in sqla.CopyToTable (#2604) #2605

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
Merged
merged 1 commit into from
Dec 18, 2018
Merged

Fix the table name when reflect is True in sqla.CopyToTable (#2604) #2605

merged 1 commit into from
Dec 18, 2018

Conversation

garaud
Copy link
Contributor
@garaud garaud commented Dec 11, 2018 •
edited
Loading

When the schema is specified, MetaData is already parametrized with the right
name. No need to call "meta.reflect" with the name of the schema in this case.

Fix the table name when reflect is True in sqla.CopyToTable (spotify#…
5c1112a 
…2604)

When the schema is specified, MetaData is already parametrized with the right
name. No need to call "
8000
meta.reflect" with the name of the schema in this case.
@garaud
Copy link
Contributor Author
garaud commented Dec 11, 2018 •
edited
Loading

I launched

tox -e core test/contrib/sqla_test.py but 0 tests was run.

@dlstadther
Copy link
Collaborator

Sqla is in the contrib tox attrib group

so:

tox -e contrib test/contrib/sqla_test.py

@dlstadther
Copy link
Collaborator

@sd2k I believe you added this bit of code. Could you review this small patch? Thanks!

@dlstadther dlstadther mentioned this pull request Dec 12, 2018
Closed
@sd2k
Copy link
Contributor
sd2k commented Dec 12, 2018 •
edited
Loading

Sure, I just tested this locally (with a Postgres DB) and it looks good (the old code failed), and it makes sense looking at the SQLAlchemy docs.

I don't think there's an easy way to add a test for it unfortunately since as far as I know SQLite (used in the tests) doesn't support schemas.

@garaud
Copy link
Contributor Author
garaud commented Dec 12, 2018

Thanks @sd2k for the review and for testing this locally.

as far as I know SQLite (used in the tests) doesn't support schemas.

I think you're right.

dlstadther
dlstadther approved these changes Dec 12, 2018
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Given above, i'm good with this

@garaud
Copy link
Contributor Author
garaud commented Dec 18, 2018

Thank you for the review. Do you think this can be merged in the future release? Thanks.

@dlstadther dlstadther merged commit eb1a2f3 into spotify:master Dec 18, 2018
@garaud
Copy link
Contributor Author
garaud commented Dec 19, 2018

Thanks!! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlstadther dlstadther dlstadther approved these changes

@Tarrasch Tarrasch Awaiting requested review from Tarrasch

@ulzha ulzha Awaiting requested review from ulzha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garaud @dlstadther @sd2k
0