[fix] Improve csv upload functionality #8457

villebro · 2019-10-26T13:55:27Z

SUMMARY

Currently all engines except Hive try to create table metadata when uploading CSVs. However, the CSV upload feature throws an exception when uploading using either replace or append when table metadata is present, despite the upload having taken place. This PR changes the following:

If table metadata is present and the CSV upload is successful, the old metadata is updated to reflect the new table contents.
Table metadata is now created for Hive like other engines when uploading CSVs.
The extension tsv added to ALLOWED_EXTENSIONS and implemented in forms.py, as I feel it is nowadays a commonly accepted extension for tab separated csv files.
Fixes to i18n CSV messages that were previously hardcoded, i.e. didn't work.
New integration tests added to test all common cases.
Refactor: parts of the CSV uploading logic had to be moved from db_engine_specs to database/views as they would otherwise have caused circular imports.

TEST PLAN

CI + Tested all possible combinations locally.

ADDITIONAL INFORMATION

Has associated issue: closes Upload CSV does not work, in replacement and append mode #8451
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

REVIEWERS

@pengyejun @willbarrett @mistercrunch @john-bodley @betodealmeida

codecov-io · 2019-10-27T06:22:09Z

Codecov Report

Merging #8457 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8457      +/-   ##
==========================================
+ Coverage   66.68%   66.72%   +0.04%     
==========================================
  Files         449      449              
  Lines       22684    22692       +8     
  Branches     2366     2366              
==========================================
+ Hits        15126    15141      +15     
+ Misses       7420     7413       -7     
  Partials      138      138

Impacted Files	Coverage Δ
superset/views/database/forms.py	`91.11% <ø> (ø)`	⬆️
superset/views/core.py	`71.96% <ø> (ø)`	⬆️
superset/config.py	`89.9% <100%> (ø)`	⬆️
superset/views/database/views.py	`92.15% <100%> (+8.64%)`	⬆️
superset/db_engine_specs/base.py	`80.33% <100%> (-0.26%)`	⬇️
superset/db_engine_specs/hive.py	`28.18% <100%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 571c474...40f1f60. Read the comment docs.

superset/views/database/forms.py

superset/views/database/views.py

superset/views/database/forms.py

villebro · 2019-11-04T16:25:37Z

@willbarrett are you ok with the latest changes? @mistercrunch @dpgaspar comments warmly welcomed!

willbarrett · 2019-11-04T23:35:51Z

superset/db_engine_specs/hive.py

@@ -98,7 +98,7 @@ def fetch_data(cls, cursor, limit: int) -> List[Tuple]:
            return []

    @classmethod
-    def create_table_from_csv(cls, form, table):  # pylint: disable=too-many-locals
+    def create_table_from_csv(cls, form):  # pylint: disable=too-many-locals


This change will make the API for the Hive db_engine_spec diverge further from base. It would behoove us to keep them in sync.

Based on my understanding of the HiveEngineSpec.create_table_from_csv, this actually aligns the functionality with that of BaseEngineSpec, as previously the Hive spec didn't create any metadata for new tables (all the others did). After this change metadata creation takes place outside of the engine spec, and works similarly for all engines, i.e. metadata is created in database/views.py, and only uploading of actual data is handled by the engine specs.

Hey Ville - the implementation is fine here, I'm only referring to the removal of the table argument from the method signature. Sorry, I should have been more specific.

@willbarrett I have tried to make sure the API remains compatible, i.e. both BaseEngineSpec ja HiveEngineSpec now only take form as additional parameters (I did forget to add the None return type to Hive, though). So there shouldn't be any discrepancy between them. Or did I misunderstand something?

superset/views/database/views.py

willbarrett

A couple more comments on the implementation.

superset/views/database/views.py

dpgaspar

Just a small comment, but it LGTM

villebro · 2019-11-06T06:24:23Z

Thanks @willbarrett and @dpgaspar for additional comments; I added a return type to the Hive upload method and replaced first() with one_or_none() in database/views.py, I think this should be ok now.

* [fix] csv upload when table metadata present * Remove table from hive spec * Move upload before table metadata creation * Refine upload logic, dd unit tests and fix translations * Use ALLOWED_EXTENSIONS from config * Address review comments * Fix error message grammar * Add return type to hive csv upload and replace first with one_or_none

pull-request-size bot added the size/M label Oct 26, 2019

pull-request-size bot added size/L and removed size/M labels Oct 27, 2019

villebro changed the title ~~[fix] csv upload when table metadata present~~ [fix] Improve csv upload functionality Oct 27, 2019

villebro mentioned this pull request Oct 27, 2019

Upload CSV does not work, in replacement and append mode #8451

Closed

willbarrett reviewed Oct 28, 2019

View reviewed changes

superset/views/database/forms.py Outdated Show resolved Hide resolved

willbarrett reviewed Oct 28, 2019

View reviewed changes

superset/views/database/views.py Outdated Show resolved Hide resolved

willbarrett reviewed Oct 29, 2019

View reviewed changes

superset/views/database/forms.py Outdated Show resolved Hide resolved

villebro requested review from dpgaspar and mistercrunch November 4, 2019 16:23

willbarrett reviewed Nov 4, 2019

View reviewed changes

superset/views/database/views.py Show resolved Hide resolved

willbarrett reviewed Nov 4, 2019

View reviewed changes

villebro added 7 commits November 5, 2019 22:35

[fix] csv upload when table metadata present

0854af2

Remove table from hive spec

d771bfe

Move upload before table metadata creation

d29252c

Refine upload logic, dd unit tests and fix translations

951cefa

Use ALLOWED_EXTENSIONS from config

c36d65b

Address review comments

7a5f12a

Fix error message grammar

c0638f9

dpgaspar reviewed Nov 6, 2019

View reviewed changes

superset/views/database/views.py Outdated Show resolved Hide resolved

dpgaspar approved these changes Nov 6, 2019

View reviewed changes

Add return type to hive csv upload and replace first with one_or_none

40f1f60

villebro merged commit 49ea232 into apache:master Nov 7, 2019

axuew mentioned this pull request Nov 12, 2019

Upload File By SuperSet Fail ! #8539

Closed

3 tasks

dpgaspar mentioned this pull request Nov 22, 2019

Problem uploading CSV to PostgreSQL database #8542

Closed

3 tasks

dpgaspar added the v0.35 label Dec 20, 2019

mistercrunch added 🍒 0.35.1 🍒 0.35.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Improve csv upload functionality #8457

[fix] Improve csv upload functionality #8457

villebro commented Oct 26, 2019 •

edited

Loading

codecov-io commented Oct 27, 2019 •

edited

Loading

villebro commented Nov 4, 2019

willbarrett Nov 4, 2019

villebro Nov 5, 2019

willbarrett Nov 5, 2019

villebro Nov 6, 2019

willbarrett left a comment

dpgaspar left a comment

villebro commented Nov 6, 2019

[fix] Improve csv upload functionality #8457

[fix] Improve csv upload functionality #8457

Conversation

villebro commented Oct 26, 2019 • edited Loading

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented Oct 27, 2019 • edited Loading

Codecov Report

villebro commented Nov 4, 2019

willbarrett Nov 4, 2019

Choose a reason for hiding this comment

villebro Nov 5, 2019

Choose a reason for hiding this comment

willbarrett Nov 5, 2019

Choose a reason for hiding this comment

villebro Nov 6, 2019

Choose a reason for hiding this comment

willbarrett left a comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

villebro commented Nov 6, 2019

villebro commented Oct 26, 2019 •

edited

Loading

codecov-io commented Oct 27, 2019 •

edited

Loading