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

Type filter in Dataset CRUD view doesn't work #11266

Closed
etr2460 opened this issue Oct 14, 2020 · 9 comments · Fixed by #11452
Closed

Type filter in Dataset CRUD view doesn't work #11266

etr2460 opened this issue Oct 14, 2020 · 9 comments · Fixed by #11452
Assignees
Labels
!deprecated-label:bug Deprecated label - Use #bug instead

Comments

@etr2460
Copy link
Member

etr2460 commented Oct 14, 2020

When I select "Physical" as the dataset type I still get virtual datasets

Expected results

Only get physical dataset types

Actual results

Also get virtual datasets

Screenshots

image

@etr2460 etr2460 added the !deprecated-label:bug Deprecated label - Use #bug instead label Oct 14, 2020
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label #bug to this issue, with a confidence of 0.95. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@dpgaspar
Copy link
Member

@etr2460 I'm not being able to reproduce this. The filter applies to is_sqllab_view

@nytai
Copy link
Member

nytai commented Oct 14, 2020

I'm also unable to reproduce. @etr2460 are some of these datasets using the native druid connector? It's possible the logic that determines the datasets type is incorrectly implemented for druid datasources. As @dpgaspar mentioned the actual filtering logic is applied to the is_sqllab_view attribute.

@nytai nytai added the need:more-info Requires more information from author label Oct 14, 2020
@etr2460
Copy link
Member Author

etr2460 commented Oct 27, 2020

@dpgaspar, @nytai: I can still repro this, even with Presto datasources. However, it looks like the ones that get filtered as Physical but are labeled as Virtual have custom sql added to them. Perhaps that needs to be part of the filtering logic too?

@nytai
Copy link
Member

nytai commented Oct 27, 2020

@etr2460 is the issue here that physical datasets are incorrectly labelled as virtual, or that the filter logic is not accounting for other instances/types of virtual datasets?

@etr2460
Copy link
Member Author

etr2460 commented Oct 27, 2020

I think it's the latter. The datasets seem to be correctly labeled as Virtual, but they still show up when you filter to Physical only datasets

@nytai
Copy link
Member

nytai commented Oct 28, 2020

ok, I see what the discrepancy is here. The label of "Physical" and "Virtual" is based on the presence of the sql attribute (see https://github.com/apache/incubator-superset/blob/155b5edec131d71eb392ee52f9bf97de495c2ef0/superset/connectors/base/models.py#L116). Meanwhile the filter for "Physical" and "Virtual" is based on the is_sqllab_view flag (see https://github.com/apache/incubator-superset/blob/155b5edec131d71eb392ee52f9bf97de495c2ef0/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx#L353).

So the question is, which attribute determines if it's a "virtual" dataset? The presence of the sql attribute or the is_sqllab_view attribute?

@nytai nytai removed the need:more-info Requires more information from author label Oct 28, 2020
@nytai
Copy link
Member

nytai commented Oct 28, 2020

hmmm, looks like is_sqllab_view only exists for sqla datasources

@nytai
Copy link
Member

nytai commented Oct 29, 2020

@etr2460 can you confirm this was fixed by #11452 when you get a chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!deprecated-label:bug Deprecated label - Use #bug instead
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants