Latest import csv #3533

timifasubaa · 2017-09-26T22:23:38Z

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. All feedback to make the code and user experience better is welcome.

@mistercrunch

…fields.

…parate upload page.

coveralls · 2017-10-06T22:36:14Z

Coverage decreased (-0.6%) to 69.481% when pulling 77158dcea97038685bf190904dfbf423003ae3f7 on timifasubaa:latest_import_csv into 6f1351f on apache:master.

coveralls · 2017-10-06T22:36:14Z

Coverage decreased (-0.6%) to 69.481% when pulling 77158dcea97038685bf190904dfbf423003ae3f7 on timifasubaa:latest_import_csv into 6f1351f on apache:master.

coveralls · 2017-10-06T22:36:14Z

Coverage decreased (-0.6%) to 69.481% when pulling 77158dcea97038685bf190904dfbf423003ae3f7 on timifasubaa:latest_import_csv into 6f1351f on apache:master.

coveralls · 2017-10-07T05:18:39Z

Coverage decreased (-0.6%) to 69.481% when pulling e35f3d21485127adb851928c92bdc85067446301 on timifasubaa:latest_import_csv into 6f1351f on apache:master.

coveralls · 2017-10-07T05:18:39Z

Coverage decreased (-0.6%) to 69.481% when pulling e35f3d21485127adb851928c92bdc85067446301 on timifasubaa:latest_import_csv into 6f1351f on apache:master.

mistercrunch · 2017-10-07T20:26:20Z

superset/db_engine_specs.py

+            'if_exists': form.if_exists.data,
+            'index': form.index.data,
+            'index_label': form.index_label.data,
+            'chunksize': 10000


NIT: we like trailing commas, we should see if pylint can require it

(maybe pylint does, and flake8 doesn't)

mistercrunch · 2017-10-07T20:27:48Z

superset/db_engine_specs.py

@@ -686,6 +771,58 @@ def fetch_result_sets(cls, db, datasource_type, force=False):
        return BaseEngineSpec.fetch_result_sets(
            db, datasource_type, force=force)

+    @staticmethod
+    def upload_csv(form, table):
+        "uploads a csv file and creates a superset table in the right databse"


https://www.python.org/dev/peps/pep-0257/

mistercrunch · 2017-10-07T20:29:12Z

superset/db_engine_specs.py

+            form.csv_file.data.filename)
+        column_names = get_column_names(upload_path)
+        schema_definition =\
+            ", ".join([col_name + " STRING " for col_name in column_names])


NIT: s/col_name/s/g

Not sure what you mean here.

mistercrunch · 2017-10-07T20:30:01Z

superset/db_engine_specs.py

+            upload_path,
+            'airbnb-superset',
+            os.path.join(upload_prefix, table_name, filename))
+        sql = "CREATE EXTERNAL TABLE {table_name} ( {schema_definition} ) "\


NIT: 3rd comment about triple-quoted strings

mistercrunch · 2017-10-07T20:32:14Z

superset/translations/es/LC_MESSAGES/messages.po

@@ -7,7 +7,7 @@ msgid ""
 msgstr ""


damn dude. these .po files are still in your PR

mistercrunch · 2017-10-07T20:35:01Z

superset/views/core.py

 import pickle
 import re
 import time
 import traceback
-
+import os


NIT: when there are many imports in a module, standard lib import should be together in a group (empty line between import groups), and alpha-sorted within that group preferably

organizing imports becomes more important as there are more imports in the module

mistercrunch · 2017-10-07T20:36:10Z

superset/views/core.py

+            flash(message, 'info')
+            return redirect('/tablemodelview/list/')
+
+


NIT: one empty line too many

mistercrunch · 2017-10-07T20:36:36Z

superset/views/core.py

 import sqlalchemy as sqla

 from flask import (
    g, request, redirect, flash, Response, render_template, Markup,
-    url_for)
-from flask_appbuilder import expose
+    url_for, send_from_directory)


unused? pylint should trigger on unused imports

mistercrunch · 2017-10-07T20:38:02Z

superset/views/core.py

+        form.if_exists.data = 'append'
+        all_datasources = []
+        all_datasources = db.session.query(
+            models.Database.sqlalchemy_uri, models.Database.database_name)\


NIT: line break would be much better after the coma

mistercrunch · 2017-10-07T20:39:29Z

superset/views/core.py

+                csv_file.save(os.path.join(config['UPLOAD_FOLDER'], filename))
+                return filename
+
+        form.names.data = form.names.data.split(",") if form.names.data else 0


Seems like form.names.data should receive consistent types, not list or Int depending... Maybe [] or None?

mistercrunch · 2017-10-07T20:40:41Z

superset/views/core.py

+
+        form.names.data = form.names.data.split(",") if form.names.data else 0
+
+        database = db.session.query(models.Database)\


Made a comment on this PR about how we like the method chaining calls to be wrap across lines before.

database = ( db.session.query(models.Database) .filter_by(sqlalchemy_uri=form.data.get('con')) .one() )

mistercrunch · 2017-10-07T20:41:41Z

superset/forms.py

+
+from superset import app, db
+import superset.models.core as models
+


NIT: uneeded blank line

coveralls · 2017-10-09T17:43:57Z

Coverage decreased (-0.6%) to 69.451% when pulling 4685ec8f0dd1aaed62bb6ad77b19698af44b19ec on timifasubaa:latest_import_csv into 64ef8b1 on apache:master.

coveralls · 2017-10-09T19:52:17Z

Coverage decreased (-0.6%) to 69.461% when pulling ae6a4bb on timifasubaa:latest_import_csv into 64ef8b1 on apache:master.

coveralls · 2017-10-09T21:02:43Z

Coverage increased (+13.5%) to 83.6% when pulling 60624b6 on timifasubaa:latest_import_csv into 64ef8b1 on apache:master.

mistercrunch

We're starting to get somewhere, should we add some unit tests?

mistercrunch · 2017-10-10T04:41:44Z

superset/db_engine_specs.py

+        if not allowed_file(filename):
+            flash("Invalid file type selected.", 'alert')
+            return False
+        kwargs = {


I don't think I was super clear about the kwargs comment earlier, and the verbose boilerplate isn't that bad either (others may consider the verbose, metaprogramming-free option better).

Knowing the form object has a data dict:
http://wtforms.simplecodes.com/docs/0.6.1/forms.html#wtforms.form.Form.data

You can do something like:

form_data = form.data csv_to_df_fields = ['names', 'filepath_or_buffer', ...] kwargs = {s: form_data.get(s) for s in csv_to_df_fields} kwargs['chunksize'] = 10000 # other alterations as needed df = BaseEngineSpec.csv_to_df(**kwargs)

mistercrunch · 2017-10-10T04:45:08Z

superset/db_engine_specs.py

+        return df
+
+    @staticmethod
+    def df_to_db(**kwargs):


if you change the function signature to def df_to_db(df, table, con, schema, **kwargs): then you don't need the 5 following lines and avoid juggling with kwargs

mistercrunch · 2017-10-10T04:46:58Z

superset/db_engine_specs.py

+        del kwargs['df']
+        del kwargs['table']
+        kwargs['con'] = create_engine(kwargs['con'], echo=False)
+        df.to_sql(**kwargs)


having taken stuff out of kwargs in the signature, now here you may to pass those things explicitely as in df.to_sql(con=create_engine(con, schema=schema, echo=False), **kwargs).

It's worth taking the time to read how those unpack operators work.

mistercrunch · 2017-10-10T04:50:11Z

superset/views/core.py

+    add_columns = ['database', 'schema', 'table_name']
+
+    def form_get(self, form):
+        form.sep.data = ','


I think the normal way of setting defaults for wtforms is to do it when creating the form objects. As in:
BooleanField('name', default=True, ...)

mistercrunch · 2017-10-10T04:50:34Z

superset/views/core.py

+        form.decimal.data = '.'
+        form.error_bad_lines.data = False
+        form.if_exists.data = 'append'
+        all_datasources = db.session.query(


this one may have to stay here since you don't want to do it in module scope...

mistercrunch · 2017-10-10T05:00:21Z

superset/db_engine_specs.py

+
+        if not bucket_path:
+            logging.info("No upload bucket specified")
+            flash(


Wood and others added 30 commits November 14, 2016 13:00

Testing gitlab remote repo

71665f7

test commit

fe81595

Merge branch 'master' of https://github.com/airbnb/superset

c16bd58

Merge branch 'master' of https://github.com/airbnb/superset

668f443

Merge branch 'master' of https://github.com/airbnb/superset

6ddcb22

Merge remote-tracking branch 'origin/master' into csv-import

452e9df

Merge branch 'master' of https://github.com/airbnb/superset

87937e5

trying out various ways of uploading xls/csv with flask

3f3ab33

Merge remote-tracking branch 'origin/master' into csv-import

39c2aba

revert previous changes

9ffbf9a

Add CSV button placed on database page

dcb0f14

Changed upload csv to use same layout/template as import dashboards

a539090

Starting to add csv to db form (to expose panda api)

b5222fc

CsvToDatabaseView updated to include most of the pandas api read_csv …

38762b0

…fields.

Exposed the pandas .to_sql API in the upload csv form. Removed the se…

ae1e9d5

…parate upload page.

Merge branch 'master' of https://github.com/airbnb/superset

ff37bd7

File select part of csvtodatabaseview form

0474253

Merge branch 'master' of https://github.com/airbnb/superset

fb186f2

If cause to set the header column to row 0 if name is none

0dc844e

Merge remote-tracking branch 'origin/master' into csv-import

f0e534d

Used flask-babel lazy_gettext to ready the new strings for translation.

96ebb82

Moved Csv form from views.py to forms.py and removed unused imports

7425b65

Changed BooleanField to use BetterBooleanField

3781a60

Added FAQ entry on uploading csv

1b4d62f

Merge branch 'master' of https://github.com/airbnb/superset

8ebf1f8

Merge remote-tracking branch 'origin/master' into csv-import

38c54c3

Merge Ryan4815's fork into my fork

1623bf8

Fix merge conflict

2c7eed7

Add tests to CSV-import

3f8ecfb

Change the default of replace to append

e560c5c

timifasubaa force-pushed the latest_import_csv branch 3 times, most recently from ea84375 to e35f3d2 Compare October 7, 2017 05:03

mistercrunch reviewed Oct 7, 2017

View reviewed changes

superset/views/core.py Outdated

flash(message, 'info')

return redirect('/tablemodelview/list/')

Copy link

Member

mistercrunch Oct 7, 2017

Choose a reason for hiding this comment

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

NIT: one empty line too many

mistercrunch reviewed Oct 7, 2017

View reviewed changes

superset/forms.py Outdated

from superset import app, db

import superset.models.core as models

Copy link

Member

mistercrunch Oct 7, 2017

Choose a reason for hiding this comment

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

NIT: uneeded blank line

timifasubaa force-pushed the latest_import_csv branch from e35f3d2 to 4685ec8 Compare October 9, 2017 17:28

timifasubaa force-pushed the latest_import_csv branch 3 times, most recently from 6df5191 to 47c0e4b Compare October 9, 2017 18:59

cleanup

ae6a4bb

timifasubaa force-pushed the latest_import_csv branch from 47c0e4b to ae6a4bb Compare October 9, 2017 19:10

Merge remote-tracking branch 'upstream/master' into latest_import_csv

60624b6

mistercrunch requested changes Oct 10, 2017

View reviewed changes

mistercrunch reviewed Oct 10, 2017

View reviewed changes

mistercrunch mentioned this pull request Oct 11, 2017

Import CSV #3643

Merged

timifasubaa closed this Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Latest import csv #3533

Latest import csv #3533

timifasubaa commented Sep 26, 2017 •

edited

Loading

coveralls commented Oct 6, 2017

coveralls commented Oct 6, 2017

coveralls commented Oct 6, 2017 •

edited

Loading

coveralls commented Oct 7, 2017 •

edited

Loading

coveralls commented Oct 7, 2017

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017

timifasubaa Oct 9, 2017

mistercrunch Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017

timifasubaa Oct 9, 2017

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017

mistercrunch Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017

coveralls commented Oct 9, 2017 •

edited

Loading

coveralls commented Oct 9, 2017 •

edited

Loading

coveralls commented Oct 9, 2017 •

edited

Loading

mistercrunch left a comment

mistercrunch Oct 10, 2017

mistercrunch Oct 10, 2017

mistercrunch Oct 10, 2017 •

edited

Loading

mistercrunch Oct 10, 2017

mistercrunch Oct 10, 2017

timifasubaa Oct 10, 2017

mistercrunch Oct 10, 2017

timifasubaa Oct 10, 2017

		flash(message, 'info')
		return redirect('/tablemodelview/list/')


		form.names.data = form.names.data.split(",") if form.names.data else 0

		database = db.session.query(models.Database)\


		from superset import app, db
		import superset.models.core as models

Latest import csv #3533

Latest import csv #3533

Conversation

timifasubaa commented Sep 26, 2017 • edited Loading

coveralls commented Oct 6, 2017

coveralls commented Oct 6, 2017

coveralls commented Oct 6, 2017 • edited Loading

coveralls commented Oct 7, 2017 • edited Loading

coveralls commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 9, 2017 • edited Loading

coveralls commented Oct 9, 2017 • edited Loading

coveralls commented Oct 9, 2017 • edited Loading

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Sep 26, 2017 •

edited

Loading

coveralls commented Oct 6, 2017 •

edited

Loading

coveralls commented Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017 •

edited

Loading

mistercrunch Oct 7, 2017 •

edited

Loading

coveralls commented Oct 9, 2017 •

edited

Loading

coveralls commented Oct 9, 2017 •

edited

Loading

coveralls commented Oct 9, 2017 •

edited

Loading

mistercrunch Oct 10, 2017 •

edited

Loading