fix(dataset): create ES-View dataset raise exception #16623 #16624

aniaan · 2021-09-07T17:35:47Z

SUMMARY

Fix the error of creating es-view dataset

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:

after:

TESTING INSTRUCTIONS

Manually create a dataset of ES-View type to see if it is successfully created. The same is true for view types of other databases, but because of the limited conditions, I only tested ES + MySQL.

ADDITIONAL INFORMATION

Has associated issue: Error creating Elasticsearch Dataset, raise NoSuchTable Exception #16623
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

zhaoyongjie · 2021-09-09T04:39:52Z

Hi @aniaan, Thanks for the fix. Could you fix failed CI?

zhaoyongjie · 2021-09-09T04:50:42Z

superset/connectors/sqla/utils.py

@@ -43,7 +43,7 @@ def get_physical_table_metadata(
    # ensure empty schema
    _schema_name = schema_name if schema_name else None
    # Table does not exist or is not visible to a connection.
-    if not database.has_table_by_name(table_name, schema=_schema_name):
+    if not database.has_table_or_view_by_name(table_name, schema=_schema_name):


Could we decouple has_table_by_name and has_view_by_name?

if any([ database.has_table_by_name(table_name, schema=_schema_name), database.has_view_by_name(table_name, schema=_schema_name), ]): ....

zhaoyongjie · 2021-09-09T05:14:49Z

superset/db_engine_specs/base.py

+        view_name: str,
+        schema: Optional[str] = None,
+    ):
+        view_names = dialect.get_view_names(connection=conn, schema=schema)


should catch NotImplementedError here

I fixed, please review

zhaoyongjie · 2021-09-09T05:17:08Z

superset/db_engine_specs/base.py

+        return view_name in view_names
+
+    @classmethod
+    def has_view(cls, engine: Engine, view_name: str, schema: Optional[str] = None):


Could we move has_view to superset.models.core in order to follow has_table pattern.

zhaoyongjie · 2021-09-09T05:17:49Z

superset/models/core.py

+            engine=engine, view_name=view_name, schema=schema
+        )
+
+    def has_table_or_view_by_name(


same before, decouple has_table_by_name and has_view_by_name

dpgaspar

Would be nice to add a test case for this, probably specific to ES mocking the API

dpgaspar · 2021-09-09T15:03:33Z

superset/models/core.py

+    ) -> bool:
+        view_names: List[str] = []
+        try:
+            view_names = dialect.get_view_names(connection=conn, schema=schema)


bugs me that we were only checking for has_table since this should just check for tables, the base dialect class on SQLAlchemy:

def has_table(self, connection, table_name, schema=None): """Check the existence of a particular table in the database. Given a :class:`.Connection` object and a string `table_name`, return True if the given table (possibly within the specified `schema`) exists in the database, False otherwise. """ raise NotImplementedError()

So this seems like a valid solution to me, kind of surprises me that this not happen on other engines.
@villebro @betodealmeida what do you think?

I tried to add test cases. Please review. The problem is essentially that there is no judgment on the view, but I found the problem when I happened to use ES, so I think the test case should be judged for the general view. Not just ES

codecov · 2021-09-09T15:20:30Z

Codecov Report

Merging #16624 (1f3d086) into master (519baa6) will increase coverage by 0.16%.
The diff coverage is 81.95%.

❗ Current head 1f3d086 differs from pull request most recent head ea948de. Consider uploading reports for the commit ea948de to get more accurate results

@@            Coverage Diff             @@
##           master   #16624      +/-   ##
==========================================
+ Coverage   76.77%   76.94%   +0.16%     
==========================================
  Files        1003     1007       +4     
  Lines       53959    54125     +166     
  Branches     7330     7346      +16     
==========================================
+ Hits        41427    41646     +219     
+ Misses      12293    12239      -54     
- Partials      239      240       +1

Flag	Coverage Δ
hive	`81.26% <86.56%> (+0.14%)`	⬆️
mysql	`81.69% <86.36%> (+0.15%)`	⬆️
postgres	`81.75% <86.56%> (+0.14%)`	⬆️
presto	`81.56% <87.31%> (+0.11%)`	⬆️
python	`82.26% <87.31%> (+0.15%)`	⬆️
sqlite	`81.32% <86.36%> (+0.11%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/Menu/Menu.tsx	`69.79% <ø> (ø)`
superset-frontend/src/dashboard/actions/hydrate.js	`1.69% <0.00%> (-0.03%)`	⬇️
...et-frontend/src/dashboard/components/Dashboard.jsx	`78.84% <ø> (ø)`
...src/dashboard/components/gridComponents/Column.jsx	`87.87% <ø> (ø)`
...nd/src/dashboard/components/gridComponents/Row.jsx	`86.66% <ø> (ø)`
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`72.22% <ø> (-1.39%)`	⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts	`56.25% <ø> (ø)`
...perset-frontend/src/dashboard/util/isValidChild.ts	`85.71% <ø> (-0.96%)`	⬇️
superset-frontend/src/views/App.tsx	`0.00% <0.00%> (ø)`
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`44.13% <0.00%> (-0.12%)`	⬇️
... and 50 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 519baa6...ea948de. Read the comment docs.

dpgaspar

LGTM. Thank you for the fix @aniaan

zhaoyongjie

LGTM

…ache#16624) * fix(dataset): create es-view dataset raise exception apache#16623 * fix(database): fix has_view logic * refactor(database): fix logic * style(lint): remove unused typing * fix(test): add test case * fix(test): fix test case

fix(dataset): create es-view dataset raise exception apache#16623

7f40cb3

pull-request-size bot added the size/S label Sep 7, 2021

aniaan changed the title ~~fix(dataset): create es-view dataset raise exception #16623~~ fix(dataset): create ES-View dataset raise exception #16623 Sep 7, 2021

fix(database): fix has_view logic

b594531

pull-request-size bot added size/M and removed size/S labels Sep 8, 2021

suddjian requested review from betodealmeida and dpgaspar September 9, 2021 00:44

zhaoyongjie reviewed Sep 9, 2021

View reviewed changes

aniaan added 2 commits September 9, 2021 14:40

refactor(database): fix logic

41b7c9e

style(lint): remove unused typing

f8f3169

aniaan requested a review from zhaoyongjie September 9, 2021 13:05

dpgaspar reviewed Sep 9, 2021

View reviewed changes

fix(test): add test case

8945f5e

aniaan requested a review from dpgaspar September 13, 2021 18:14

fix(test): fix test case

ea948de

dpgaspar approved these changes Sep 14, 2021

View reviewed changes

zhaoyongjie approved these changes Sep 14, 2021

View reviewed changes

zhaoyongjie merged commit 9e00e4e into apache:master Sep 14, 2021

dpgaspar mentioned this pull request Sep 14, 2021

update has_table to accept view or table preset-io/elasticsearch-dbapi#66

Closed

aniaan deleted the fix/GH16623 branch October 11, 2021 02:52

This was referenced May 9, 2022

Cannot create a Dataset of an Exasol Database View #16541

Closed

Discussion regarding has_table method for views exasol/sqlalchemy-exasol#109

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dataset): create ES-View dataset raise exception #16623 #16624

fix(dataset): create ES-View dataset raise exception #16623 #16624

aniaan commented Sep 7, 2021 •

edited

Loading

zhaoyongjie commented Sep 9, 2021

zhaoyongjie Sep 9, 2021

zhaoyongjie Sep 9, 2021

aniaan Sep 9, 2021

zhaoyongjie Sep 9, 2021

zhaoyongjie Sep 9, 2021

dpgaspar left a comment

dpgaspar Sep 9, 2021 •

edited

Loading

aniaan Sep 13, 2021

codecov bot commented Sep 9, 2021 •

edited

Loading

dpgaspar left a comment •

edited

Loading

zhaoyongjie left a comment

fix(dataset): create ES-View dataset raise exception #16623 #16624

fix(dataset): create ES-View dataset raise exception #16623 #16624

Conversation

aniaan commented Sep 7, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

zhaoyongjie commented Sep 9, 2021

zhaoyongjie Sep 9, 2021

Choose a reason for hiding this comment

zhaoyongjie Sep 9, 2021

Choose a reason for hiding this comment

aniaan Sep 9, 2021

Choose a reason for hiding this comment

zhaoyongjie Sep 9, 2021

Choose a reason for hiding this comment

zhaoyongjie Sep 9, 2021

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

aniaan Sep 13, 2021

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2021 • edited Loading

Codecov Report

dpgaspar left a comment • edited Loading

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

aniaan commented Sep 7, 2021 •

edited

Loading

dpgaspar Sep 9, 2021 •

edited

Loading

codecov bot commented Sep 9, 2021 •

edited

Loading

dpgaspar left a comment •

edited

Loading