feat: support nulls in the csv uploads #10208

bkyryliuk · 2020-06-30T18:29:44Z

SUMMARY

There are use cases when users would prefer to treat empty strings in the csv as nulls in the database.
This PR makes the field to be configurable and bring more transparency to how it is handled.
A default value is set to be the same as pandas, it doesn't look pretty and probably has too many values, that's why I also made it configurable.

Default values:

In addition to the feature, I've refactored hive_tests.py to follow pytest standards, now they can be run directly via pytest command.

TEST PLAN

pytest -s tests/db_engine_specs/hive_tests.py - for hive changes
tox -e py36 -- tests/core_tests.py::TestCore::test_import_csv - for general changes
tested locally for different uses case on mysql
tested on our staging env for hive changes

Hive testing screenshots

ADDITIONAL INFORMATION

codecov-commenter · 2020-07-01T05:58:08Z

Codecov Report

Merging #10208 into master will decrease coverage by 6.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10208      +/-   ##
==========================================
- Coverage   65.70%   59.44%   -6.26%     
==========================================
  Files         594      404     -190     
  Lines       31501    13107   -18394     
  Branches     3221     3221              
==========================================
- Hits        20697     7792   -12905     
+ Misses      10623     5134    -5489     
  Partials      181      181

Flag	Coverage Δ
#javascript	`59.44% <ø> (ø)`
#python	`?`

Impacted Files	Coverage Δ
superset/dao/base.py
superset/exceptions.py
superset/db_engine_specs/redshift.py
superset/dao/exceptions.py
superset/tasks/celery_app.py
superset/db_engine_specs/pinot.py
superset/views/dashboard/mixin.py
superset/examples/multi_line.py
superset/tasks/slack_util.py
superset/views/chart/filters.py
... and 175 more

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 4281ad5...dba2d9b. Read the comment docs.

bkyryliuk · 2020-07-01T20:08:03Z

tests/db_engine_specs/hive_tests.py

-            {},
-            {"if_exists": "append"},
-        )
+def test_0_progress():


refactored to be pytest friendly

serenajiang

mostly lgtm

Are integer columns affected by this change at all? I'm not sure how we deal with null integers in csv upload.

serenajiang · 2020-07-01T22:26:10Z

superset/db_engine_specs/hive.py

+            tblproperties.append(f"'serialization.null.format'='{null_values[0]}'")
+        tblproperties_stmt = ""
+        if tblproperties:
+            tblproperties_stmt = f"tblproperties ({', '.join(tblproperties)})"


Can we write this in some way that lets us use :params for the table properties?

issue is that tblproperties is optional - and it makes it quite tricky to implement, I did not figure it out. Open to ideas here

Maybe we could make this function return Tuple[text, Dict[str, str]]. The code in the outer function would be something like

sql, params = cls.get_create_table_stmt(...) ... engine.execute(sql, **params)

@serenajiang done

serenajiang · 2020-07-01T22:42:31Z

superset/forms.py

+    def _value(self) -> str:
+        return json.dumps(self.data)
+
+    def process_formdata(self, valuelist: List[str]) -> None:


Just curious - what happens if the user passes in a malformed input?

it will not allow to submit the form, error message is quite cryptic, that's why description has examples

bkyryliuk · 2020-07-01T23:16:41Z

mostly lgtm

Are integer columns affected by this change at all? I'm not sure how we deal with null integers in csv upload.

Thanks @serenajiang for the review
Mysql recognized the column as double and wrote null

Hive:
int + null value

bkyryliuk · 2020-07-02T15:35:49Z

pytest.ini

@@ -15,7 +15,6 @@
 # limitations under the License.
 #
 [pytest]
-addopts = -ra -q


cleaned it up to be able to use -vv flag for the verbose output

etr2460 · 2020-07-02T17:05:28Z

Maybe this isn't easy/possible with WTForms, but is there a way we can structure this new input so that instead of passing in a JSON encoded array, we can add individual items and they'll render as individual inputs that then get parsed correctly by the backend? Something similar to the owner fields in the crud view:

bkyryliuk · 2020-07-02T17:39:18Z

Maybe this isn't easy/possible with WTForms, but is there a way we can structure this new input so that instead of passing in a JSON encoded array, we can add individual items and they'll render as individual inputs that then get parsed correctly by the backend? Something similar to the owner fields in the crud view:

@etr2460 good suggestion, I've explored FieldList but wasn't able to get it work. Owners are built using many to many ORM mapping, it wouldn't fit this use case. Hopefully this form will be shortlived and migrated to react in some foreseeable future.

villebro · 2020-07-03T05:51:47Z

superset/views/database/forms.py

@@ -206,3 +210,13 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
        validators=[Optional()],
        widget=BS3TextFieldWidget(),
    )
+    null_values = JsonListField(
+        _("Null values"),
+        default=config.get("CSV_DEFAULT_NA_NAMES", []),


This should be config["CSV_DEFAULT_NA_NAMES"]

villebro

Small nit, other than that LGTM. Also needs a rebase.

Refactor Add tests, and refactor them to be pytest friendly Use lowercase table names Ignore isort

bkyryliuk · 2020-07-06T20:26:40Z

Small nit, other than that LGTM. Also needs a rebase.

thanks for the review!

* Support more table properties for the hive upload Refactor Add tests, and refactor them to be pytest friendly Use lowercase table names Ignore isort * Use sql params Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>

pull-request-size bot added the size/L label Jun 30, 2020

bkyryliuk marked this pull request as draft June 30, 2020 18:29

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch 6 times, most recently from 6de52fa to 3e3927c Compare July 1, 2020 05:42

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch 2 times, most recently from 4d3b949 to 0033d1a Compare July 1, 2020 19:08

bkyryliuk requested review from serenajiang, villebro, john-bodley and etr2460 July 1, 2020 20:05

bkyryliuk changed the title ~~feat: support nulls in the csv uploads [WIP]~~ feat: support nulls in the csv uploads Jul 1, 2020

bkyryliuk marked this pull request as ready for review July 1, 2020 20:06

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from 0033d1a to d6c0a4b Compare July 1, 2020 20:06

bkyryliuk commented Jul 1, 2020

View reviewed changes

tests/db_engine_specs/hive_tests.py

{},

{"if_exists": "append"},

)

def test_0_progress():

Copy link

Member Author

bkyryliuk Jul 1, 2020

Choose a reason for hiding this comment

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

refactored to be pytest friendly

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from d6c0a4b to 3201703 Compare July 1, 2020 22:29

serenajiang reviewed Jul 1, 2020

View reviewed changes

pull-request-size bot added size/XL and removed size/L labels Jul 2, 2020

bkyryliuk commented Jul 2, 2020

View reviewed changes

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch 2 times, most recently from cdde17c to 08143e0 Compare July 2, 2020 15:50

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from 08143e0 to a225116 Compare July 2, 2020 17:40

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from a225116 to 63d502a Compare July 2, 2020 17:46

bkyryliuk added the v0.37 label Jul 2, 2020

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from 63d502a to dba2d9b Compare July 2, 2020 17:56

villebro reviewed Jul 3, 2020

View reviewed changes

bogdan-dbx added 2 commits July 6, 2020 10:53

Support more table properties for the hive upload

25e98d9

Refactor Add tests, and refactor them to be pytest friendly Use lowercase table names Ignore isort

Use sql params

3881fac

bkyryliuk force-pushed the bogdan/support_nulls_in_hive branch from dba2d9b to 3881fac Compare July 6, 2020 17:53

bkyryliuk merged commit 84f8a51 into apache:master Jul 6, 2020

bkyryliuk deleted the bogdan/support_nulls_in_hive branch July 6, 2020 20:26

serenajiang mentioned this pull request Jul 9, 2020

fix(csv upload): hive params typo #10273

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support nulls in the csv uploads #10208

feat: support nulls in the csv uploads #10208

bkyryliuk commented Jun 30, 2020 •

edited

codecov-commenter commented Jul 1, 2020 •

edited

bkyryliuk Jul 1, 2020

serenajiang left a comment

serenajiang Jul 1, 2020

bkyryliuk Jul 1, 2020

serenajiang Jul 1, 2020

bkyryliuk Jul 2, 2020

serenajiang Jul 1, 2020

bkyryliuk Jul 1, 2020

bkyryliuk commented Jul 1, 2020

bkyryliuk Jul 2, 2020

etr2460 commented Jul 2, 2020

bkyryliuk commented Jul 2, 2020

villebro Jul 3, 2020

villebro left a comment

bkyryliuk commented Jul 6, 2020

feat: support nulls in the csv uploads #10208

feat: support nulls in the csv uploads #10208

Conversation

bkyryliuk commented Jun 30, 2020 • edited

SUMMARY

TEST PLAN

Hive testing screenshots

ADDITIONAL INFORMATION

codecov-commenter commented Jul 1, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

serenajiang left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Jul 1, 2020

Choose a reason for hiding this comment

etr2460 commented Jul 2, 2020

bkyryliuk commented Jul 2, 2020

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

bkyryliuk commented Jul 6, 2020

bkyryliuk commented Jun 30, 2020 •

edited

codecov-commenter commented Jul 1, 2020 •

edited