-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat: create dtype option for csv upload #23716
feat: create dtype option for csv upload #23716
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23716 +/- ##
==========================================
+ Coverage 65.96% 67.98% +2.01%
==========================================
Files 1907 1936 +29
Lines 73590 74928 +1338
Branches 7982 8140 +158
==========================================
+ Hits 48546 50942 +2396
+ Misses 22996 21894 -1102
- Partials 2048 2092 +44
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 332 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2472ebe
to
2bed125
Compare
2bed125
to
e76b921
Compare
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 looks great!
superset/views/database/forms.py
Outdated
dtype = StringField( | ||
_("Column Data Types"), | ||
description=_( | ||
"A dictionary with column names and their data types if you need to change the defaults. Example: {“Column”:“data 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.
I would use a concrete example here, something like:
"A dictionary with column names and their data types if you need to change the defaults. Example: {“Column”:“data type”}" | |
"A dictionary with column names and their data types if you need to change the defaults. Example: {“user_id”:“integer”}" |
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.
good idea!
322178e
to
8055592
Compare
8055592
to
17792e2
Compare
bd36bce
to
ef00cd0
Compare
SUMMARY
Redshift automatically converts all text columns to a varchar(256) which means for uploads, and in particular csv uploads for this case, the only way to upload a column with a large text field is to first create the table in sql lab with the correct column definitions and then upload to that existing table and replace the data. This pr adds a new dtype field for the upload, and using the "string" property, converts any field of this type to a varchar(max) for redshift. Currently all string types are uploaded as "object", so this change shouldn't impact any uploads that aren't explicitly passing the "string" property. We could extend this to other dbs, but it looks like the default behavior is for uploaded string columns to be converted to
text
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After: (added a new field)
In this example, I updated just the column type to nvarchar(max) by passing
{"Question":"string"}
into the upload form. The other string field remains as varchar(256).TESTING INSTRUCTIONS
ADDITIONAL INFORMATION