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

Import CSV #3643

Merged
merged 10 commits into from
Nov 28, 2017
Merged

Import CSV #3643

merged 10 commits into from
Nov 28, 2017

Conversation

timifasubaa
Copy link
Contributor

This PR allows users to import CSV files to superset. It extends the work that had already been done in #2381 and also adds the new functionality of uploading to a hive datasource.

@coveralls
Copy link

coveralls commented Oct 10, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 69.514% when pulling 94fcd20fe7b5f38fd871db7dfcba45d9717f7303 on timifasubaa:import_csv into d7f8a7f on apache:master.

@mistercrunch
Copy link
Member

How does this relate to #3533 ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 69.967% when pulling 62df140ffd0e0cb5f9a42d74a6927aac50f51b2f on timifasubaa:import_csv into d7f8a7f on apache:master.

@timifasubaa timifasubaa force-pushed the import_csv branch 2 times, most recently from 8d392dc to eb0ad94 Compare October 13, 2017 01:26
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 70.052% when pulling eb0ad94fa9c350910b3a5d12d9174f30ef286f0c on timifasubaa:import_csv into f871634 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 70.052% when pulling eb0ad94fa9c350910b3a5d12d9174f30ef286f0c on timifasubaa:import_csv into f871634 on apache:master.

@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 70.321% when pulling 3ba69ff7f4c2f7007391141627464fbfc186f630 on timifasubaa:import_csv into f871634 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 70.052% when pulling b1a3df8a9767927d8f7dce8f144368c399fd1e13 on timifasubaa:import_csv into f871634 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 70.052% when pulling b1a3df8a9767927d8f7dce8f144368c399fd1e13 on timifasubaa:import_csv into f871634 on apache:master.

@mistercrunch
Copy link
Member

Build doesn't pass...

@timifasubaa
Copy link
Contributor Author

The "Not a valid choice" error is coming up for postgres and mysql dbs (the 2 failing toxenvs). Still can't reproduce the error.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 5a36884387df52e006852ca1ffb1959cadb2eb93 on timifasubaa:import_csv into bad6938 on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 5a36884387df52e006852ca1ffb1959cadb2eb93 on timifasubaa:import_csv into bad6938 on apache:master.

@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 5a36884387df52e006852ca1ffb1959cadb2eb93 on timifasubaa:import_csv into bad6938 on apache:master.

@timifasubaa timifasubaa force-pushed the import_csv branch 3 times, most recently from c85b508 to 11f1238 Compare October 13, 2017 21:19
@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 70.444% when pulling c85b50856ab1fb2a1dc5f9576f598a2ba1945f4c on timifasubaa:import_csv into bad6938 on apache:master.

@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 70.444% when pulling 11f12389d5ad0c36bfcad196069427dcf890d3c0 on timifasubaa:import_csv into bad6938 on apache:master.

@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 8b68fb01f74381285c6c98ab6c4e19a66147cfed on timifasubaa:import_csv into bad6938 on apache:master.

@timifasubaa
Copy link
Contributor Author

@mistercrunch resolved!

@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 91a0e1d1c37ca9d267110c4579d3926cfcedd874 on timifasubaa:import_csv into bad6938 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 70.584% when pulling 91a0e1d1c37ca9d267110c4579d3926cfcedd874 on timifasubaa:import_csv into bad6938 on apache:master.

@timifasubaa
Copy link
Contributor Author

@mistercrunch All merge conflicts have been resolved and the build passes again. Can we have this PR reviewed soon and then I can make a follow up PR that adds permissions.

john-bodley
john-bodley reviewed Nov 21, 2017
View reviewed changes
superset/db_engine_specs.py Outdated

@staticmethod
def create_table_from_csv(form, table):
def allowed_file(filename):
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Rename allowed_file to _allowed_file`.

superset/db_engine_specs.py Outdated
logging.info(form.con.data)
engine = create_engine(form.con.data)
engine.execute(sql)
return (True, '')
Copy link
Member

Choose a reason for hiding this comment

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

It seems more Pythonic to return a scalar and raise an exception in the case of the failure.

superset/views/core.py Outdated
form.if_exists.data = 'append'
all_datasources = db.session.query(
models.Database.sqlalchemy_uri,
models.Database.database_name)\
Copy link
Member

Choose a reason for hiding this comment

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

Nit. I don't think the \ is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I removed it, I got unexpected indent.

superset/views/core.py Outdated
form.con.choices += all_datasources

def form_post(self, form):
def upload_file(csv_file):
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Rename upload_file to _upload_file.

tests/core_tests.py
@@ -786,6 +788,46 @@ def test_viz_get_fillna_for_columns(self):
{'name': ' NULL', 'sum__num': 0},
)

def test_import_csv(self):
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding unit tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

tox.ini
@@ -68,12 +68,12 @@ commands =
[testenv:py27-mysql]
basepython = python2.7
setenv =
SUPERSET__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/superset?charset=utf8
SUPERSET__SQLALCHEMY_DATABASE_URI = mysql://root@localhost/superset?charset=utf8
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing the user/password for the test databases?

Copy link
Contributor Author

@timifasubaa timifasubaa Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It didn't work with the previous credentials. Insufficient privileges for adding a table if I recall correctly.

@timifasubaa timifasubaa force-pushed the import_csv branch 9 times, most recently from d6fe866 to 4de9028 Compare November 22, 2017 00:38
@john-bodley
Copy link
Member

LGTM

@mistercrunch mistercrunch merged commit 268edcf into apache:master Nov 28, 2017
@timifasubaa timifasubaa mentioned this pull request Nov 29, 2017
Closed
@rhunwicks rhunwicks mentioned this pull request Dec 5, 2017
Closed
1 task
@TryHarder01
Copy link

dumb question... is there an obvious way to see what version this PR became available?

@frankhsu
Copy link

frankhsu commented Jan 4, 2018

@TryHarder01 Search for the PR number #3643 in the CHANGELOG.

@TryHarder01
Copy link

@frankhsu no results when I search the change log.

Not a huge deal, I was just curious how to track PRs into versions.

@mistercrunch
Copy link
Member

A release is coming soon :)

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
@timifasubaa @michellethomas
Import CSV (apache#3643)
0a81715 
* add upload csv button to sources dropdown

* upload csv to non-hive datasources

* upload csv to hive datasource

* update FAQ page

* add tests

* fix linting errors and merge conflicts

* Update .travis.yml

* Update tox.ini
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@timifasubaa @mistercrunch
Import CSV (apache#3643)
485f7c5 
* add upload csv button to sources dropdown

* upload csv to non-hive datasources

* upload csv to hive datasource

* update FAQ page

* add tests

* fix linting errors and merge conflicts

* Update .travis.yml

* Update tox.ini
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@john-bodley john-bodley john-bodley left review comments

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@timifasubaa @coveralls @mistercrunch @rpwils @john-bodley @TryHarder01 @frankhsu