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.

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

override get_view_names in PrestoEngineSpec #6459

Merged
merged 4 commits into from Nov 28, 2018

Conversation

youngyjd
Copy link
Contributor

@youngyjd youngyjd commented Nov 28, 2018

When fetching metadata for presto engine spec we got an error which says:

ts=2018-11-28T20:42:52.414Z uuid=98709625-59fd-4d9d-88e2-22471db6fec0 app=superset name=root lvlname=ERROR lvlno=40 pid=103884 request_id=3b7d72c3-4949-97a7-9749-92da9cd4e2a9 exc="Traceback (most recent call last):
File \"/srv/venvs/service/trusty/service_venv_python3.6/lib/python3.6/site-packages/superset/models/core.py\", line 947, in all_view_names_in_schema
inspector=self.inspector, schema=schema)
File \"/srv/venvs/service/trusty/service_venv_python3.6/lib/python3.6/site-packages/superset/db_engine_specs.py\", line 306, in get_view_names
return sorted(inspector.get_view_names(schema))
File \"/srv/venvs/service/trusty/service_venv_python3.6/lib/python3.6/site-packages/sqlalchemy/engine/reflection.py\", line 324, in get_view_names 
info_cache=self.info_cache)
File \"/srv/venvs/service/trusty/service_venv_python3.6/lib/python3.6/site-packages/sqlalchemy/engine/interfaces.py\", line 330, in get_view_names
raise NotImplementedError()
NotImplementedError" msg=

Then I found get_view_names() is not implemented in https://github.com/dropbox/PyHive/blob/e25fc8440a0686bbb7a5db5de7cb1a77bdb4167a/pyhive/sqlalchemy_presto.py because get_table_names() function returns all table names and view names.

For Hive, I observed that every time when superset tries to fetch table list and view list in default schema, it issues SHOW TABLES IN default twice. I found out that's because get_view_names() is indeed a call of get_table_names. Since HiveEngineSpec() inherits from PrestoEngineSpec(), we can just leverage the get_view_names() in PrestoEngineSpec().

@codecov-io
Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #6459 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6459      +/-   ##
=========================================
- Coverage   73.31%   73.3%   -0.01%     
=========================================
  Files          67      67              
  Lines        9589    9591       +2     
=========================================
+ Hits         7030    7031       +1     
- Misses       2559    2560       +1
Impacted Files Coverage Δ
superset/db_engine_specs.py 55.02% <50%> (-0.02%) ⬇️

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 4579b12...bd3ffa6. Read the comment docs.

@youngyjd
Copy link
Contributor Author

@betodealmeida

@betodealmeida
Copy link
Member

betodealmeida commented Nov 28, 2018

Looks good! Note that it should be trivial to implement get_view_names in the Presto dialect; all you need to do is run this query:

SELECT table_name FROM information_schema.views WHERE table_schema='{schema}'

Might be worth doing a PR for PyHive. ;)

@betodealmeida betodealmeida merged commit f1cae2e into apache:master Nov 28, 2018
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Nov 29, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Nov 30, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Dec 7, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Dec 7, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Dec 7, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Dec 10, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Dec 10, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Dec 10, 2018
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 11, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 11, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 11, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 11, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 12, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 14, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 17, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 24, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Jan 30, 2019
* override get_view_names in PrestoEngineSpec

* add test

* flake 8

* flake 8

(cherry picked from commit f1cae2e)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants