chore: ci Initial hive support #10593

bkyryliuk · 2020-08-13T03:16:22Z

SUMMARY

Hive tests

added hive to the CI tests via using step and docker compose for sqllab and csv upload tests
exposed UPLOAD_FOLDER to be able to mount it with the docker
hdfs for the csv uploads is used instead of s3 via @ pytest.patch

Related cleanup

reduced presto tests runtime down from 16 to 9 minutes via configuring the poll_interval
sqlalchemy tables now are dropped as a part of the cleanup
added ipdb to the test dependencies to be able to debug inside tox environment

Side effects:

somehow test_update_dataset_update_column started failing for the presto database only on CI, was not able to reproduce it locally, help here would be greatly appreciated

Followed https://hub.docker.com/r/bde2020/hive/ as an example

TEST PLAN

Mostly test only changes, CI

codecov-commenter · 2020-08-13T05:20:34Z

Codecov Report

Merging #10593 into master will decrease coverage by 4.01%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master   #10593      +/-   ##
==========================================
- Coverage   64.36%   60.35%   -4.02%     
==========================================
  Files         785      785              
  Lines       36981    36987       +6     
  Branches     3530     3530              
==========================================
- Hits        23803    22323    -1480     
- Misses      13070    14477    +1407     
- Partials      108      187      +79

Flag	Coverage Δ
#cypress	`?`
#javascript	`60.91% <ø> (ø)`
#python	`60.01% <44.44%> (+0.29%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/presto.py	`73.54% <ø> (-8.79%)`	⬇️
superset/examples/energy.py	`100.00% <ø> (ø)`
superset/examples/unicode_test_data.py	`100.00% <ø> (ø)`
superset/db_engine_specs/hive.py	`83.98% <41.17%> (+28.96%)`	⬆️
superset/config.py	`90.38% <100.00%> (+0.03%)`	⬆️
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/index.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/index.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
... and 154 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 4b40d44...ba74384. Read the comment docs.

john-bodley · 2020-08-16T02:47:22Z

requirements/testing.in

@@ -16,7 +16,10 @@
 #
 -r base.in
 -r integration.in
+ipdb
+ipython==7.16.1


Nit. ABC. Also why pin ipython?

Added comment:

# pinning ipython as pip-compile-multi was bringing higher version # of the ipython that was not found in CI

superset/db_engine_specs/hive.py

john-bodley · 2020-08-16T02:51:48Z

tests/conftest.py

@@ -14,18 +14,27 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+# isort:skip_file


Why is this necessary? I prefer the previous logic on 23.

from superset import db needs to be called after test_app import
e.g. https://github.com/apache/incubator-superset/blob/fd2d1c58c566d9312d6cfc5641a06ac2b03e753a/tests/sqllab_tests.py#L27

villebro · 2020-08-23T18:18:06Z

FYI @bkyryliuk this needs a rebase

villebro

I few minor comments, for the most part looks good!

villebro · 2020-08-23T18:18:51Z

.github/workflows/superset-python.yml

+    strategy:
+      matrix:
+        # run unit tests in multiple version just for fun
+        python-version: [3.6, 3.7, 3.8]


I'd suggest dropping 3.6 from new tests going forward.

villebro · 2020-08-23T18:20:30Z

requirements/testing.txt

+ipython-genutils==0.2.0   # via traitlets
+ipython==7.16.1           # via -r requirements/testing.in, ipdb


This is surprising, do we need this dependency?

this is for ipdb, it CI had issues locationg ipython > 7.16.1, I'll try it again.
As for ipdb - I like using it inside tox, that's why added to the test dependencies. It provides really nice debugging experience.

update github actions still are unable to install ipython 7.17.0

Nice, I'll try it out 👍

villebro · 2020-08-23T18:28:20Z

tests/csv_upload_tests.py

+    if utils.backend() == "hive":
+        # Be aware that hive only uses first value from the null values list
+        # it can be fixes if we will process csv file on the superset webserver.
+        assert data == [("john", 1, "x"), ("paul", 2, None)]
+    else:
+        assert data == [(None, 1, "x"), ("paul", 2, None)]


I didn't quite understand this, why is "john" showing up here on Hive when other dbs return None?

see the comment above, hive has it's own csv upload implementation and only supports a single null value where as default implementation works with the list. [ hive engine limitation ]

It can manually process csv to make it feature complete. Updated the comment to be more descriptive & added a todo

villebro · 2020-08-23T18:29:44Z

tests/db_engine_specs/oracle_tests.py

@@ -18,6 +18,7 @@
 from sqlalchemy.dialects import oracle
 from sqlalchemy.dialects.oracle import DATE, NVARCHAR, VARCHAR

+from superset.db_engine_specs.hive import HiveEngineSpec


I assume this isn't needed here?

good catch :)

villebro · 2020-08-23T18:32:38Z

tests/model_tests.py

-        expected = (
-            textwrap.dedent(
-                f"""\
-        SELECT source,
-               target,
-               value
-        FROM {table_name}
-        LIMIT 100"""
+        # TODO(bkyryliuk): unify sql generation
+        if db.backend == "presto":
+            assert (
+                'SELECT "source" AS "source",\n       "target" AS "target",\n       "value" AS "value"\nFROM "energy_usage"\nLIMIT 100'
+                == sql
            )
-            if db.backend != "presto"
-            else textwrap.dedent(
-                f"""\
-        SELECT "source" AS "source",
-               "target" AS "target",
-               "value" AS "value"
-        FROM "{table_name}"
-        LIMIT 100"""
+        elif db.backend == "hive":
+            assert (
+                "SELECT `source`,\n       `target`,\n       `value`\nFROM `energy_usage`\nLIMIT 100"
+                == sql
+            )
+        else:
+            assert (
+                "SELECT source,\n       target,\n       value\nFROM energy_usage\nLIMIT 100"
+                in sql


nit/personal preference: I kind of preferred the appearance of textwrap.dedent over this.

villebro

LGTM 👍

villebro · 2020-08-27T15:39:03Z

requirements/testing.txt

+ipython-genutils==0.2.0   # via traitlets
+ipython==7.16.1           # via -r requirements/testing.in, ipdb


Nice, I'll try it out 👍

* Initial hive support * Clone hive setup * Make hive tests work locally * Debugging presto failure * sleep in dataset test * Address comments * Address comments * Pin ipython, exclude new pylint rules Co-authored-by: bogdan kyryliuk <[email protected]>

pull-request-size bot added the size/M label Aug 13, 2020

bkyryliuk marked this pull request as draft August 13, 2020 03:16

bkyryliuk force-pushed the bogdan/hive_ci branch 11 times, most recently from d69e8e9 to a13658d Compare August 13, 2020 05:11

bkyryliuk force-pushed the bogdan/hive_ci branch from a13658d to d5537e1 Compare August 13, 2020 05:23

pull-request-size bot added size/L and removed size/M labels Aug 13, 2020

bkyryliuk force-pushed the bogdan/hive_ci branch 9 times, most recently from 9e42631 to 31c1647 Compare August 13, 2020 20:43

pull-request-size bot added size/XL and removed size/L labels Aug 13, 2020

bkyryliuk force-pushed the bogdan/hive_ci branch from 315a75e to 0eaf1db Compare August 13, 2020 21:10

mistercrunch added the .database:driver label Aug 13, 2020

bkyryliuk requested review from villebro and ktmud August 16, 2020 01:44

bkyryliuk changed the title ~~chore: ci Initial hive support [WIP]~~ chore: ci Initial hive support Aug 16, 2020

john-bodley reviewed Aug 16, 2020

View reviewed changes

bkyryliuk marked this pull request as ready for review August 16, 2020 17:24

villebro reviewed Aug 23, 2020

View reviewed changes

bogdan-dbx added 6 commits August 25, 2020 08:05

Initial hive support

a498f80

Clone hive setup

c4303c3

Make hive tests work locally

487f46b

Debugging presto failure

9e7d667

sleep in dataset test

15c164b

Address comments

c44cd22

bkyryliuk force-pushed the bogdan/hive_ci branch from ca0ae41 to c44cd22 Compare August 25, 2020 15:21

Address comments

8178c43

bkyryliuk force-pushed the bogdan/hive_ci branch 2 times, most recently from 5f4f914 to ba74384 Compare August 25, 2020 17:34

Pin ipython, exclude new pylint rules

66fe3a4

bkyryliuk force-pushed the bogdan/hive_ci branch from ba74384 to 66fe3a4 Compare August 25, 2020 17:56

villebro approved these changes Aug 27, 2020

View reviewed changes

bkyryliuk merged commit 19a9bcc into apache:master Aug 27, 2020

bkyryliuk deleted the bogdan/hive_ci branch August 27, 2020 16:49

dpgaspar added the v0.38 label Sep 22, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ci Initial hive support #10593

chore: ci Initial hive support #10593

bkyryliuk commented Aug 13, 2020 •

edited

Loading

codecov-commenter commented Aug 13, 2020 •

edited

Loading

john-bodley Aug 16, 2020

bkyryliuk Aug 16, 2020

john-bodley Aug 16, 2020

bkyryliuk Aug 16, 2020

villebro commented Aug 23, 2020

villebro left a comment

villebro Aug 23, 2020

bkyryliuk Aug 25, 2020

villebro Aug 23, 2020

bkyryliuk Aug 25, 2020

bkyryliuk Aug 25, 2020

villebro Aug 27, 2020

villebro Aug 23, 2020

bkyryliuk Aug 25, 2020

villebro Aug 23, 2020

bkyryliuk Aug 25, 2020

villebro Aug 23, 2020

villebro left a comment

villebro Aug 27, 2020

		ipython-genutils==0.2.0 # via traitlets
		ipython==7.16.1 # via -r requirements/testing.in, ipdb

chore: ci Initial hive support #10593

chore: ci Initial hive support #10593

Conversation

bkyryliuk commented Aug 13, 2020 • edited Loading

SUMMARY

TEST PLAN

codecov-commenter commented Aug 13, 2020 • edited Loading

Codecov Report

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

villebro commented Aug 23, 2020

villebro 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

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

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Aug 13, 2020 •

edited

Loading

codecov-commenter commented Aug 13, 2020 •

edited

Loading