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

Change task result parameter value to TEXT #2404

Merged
merged 4 commits into from
May 2, 2018

Conversation

kwilcox
Copy link
Contributor

@kwilcox kwilcox commented Apr 24, 2018

This is a manual rebase of #2241 onto master with one additional commit.

Description

Parameter values longer than 256 characters currently fail to be written to the SQL result store. This changes the task value column to TEXT to avoid the issue.

Motivation and Context

Fixes #2240
Fixes #1885

Have you tested this? If so, how?

I've tested this against a PostgreSQL setup, both a new database and existing database, and it works. I have not tested any other dialects (help plz). The existing tests should cover MySQL and SQLite? Marking WIP until I can confirm this works with the required dialects.

@kwilcox
Copy link
Contributor Author

kwilcox commented Apr 24, 2018

I tested this extensively against PostgreSQL and SQLite and it migrates things correctly if moving from a previous table on PostgreSQL. SQLite doesn't support changing column types without changing the table name and copying the data over so a warning message is logged if the migration can't be run.

@kwilcox kwilcox changed the title WIP: Change task result parameter value to TEXT Change task result parameter value to TEXT Apr 24, 2018
Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Didn't carefully read the code. But if you say it works. I'm happy to merge. Maybe let the PR be for a few days in case somebody else objects or wants to review.

@kwilcox
Copy link
Contributor Author

kwilcox commented Apr 30, 2018

@jimexist @oivoodoo would you like to confirm this is working for you before merge?

@oivoodoo
Copy link

@kwilcox it looks great. I believe that it would work. thank you!

@Tarrasch Tarrasch merged commit 9298776 into spotify:master May 2, 2018
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.

luigi parameter value column is set to limit size of 256
4 participants