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

ISSUE-60: Implement get_view_names() #62

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

dimkonko
Copy link
Contributor

@dimkonko dimkonko commented Jul 29, 2020

Issue: #60

Major change: in this change get_table_names will return only table names (without view names).

@tswast
Copy link
Collaborator

tswast commented Aug 3, 2020

Makes sense, but does Superset call get_view_names? I'm worried that dashboarding tools might not see all the tables they're intended to see if get_table_names only returns non-view tables.

@romainr
Copy link
Contributor

romainr commented Sep 30, 2020

Interesting: https://docs.sqlalchemy.org/en/13/core/reflection.html#sqlalchemy.engine.reflection.Inspector.get_table_names

The names are expected to be real tables only, not views. Views are instead returned using the Inspector.get_view_names() method.


result = []
for dataset in datasets:
if current_schema is not None and current_schema != dataset.dataset_id:
Copy link
Collaborator

@tswast tswast Nov 18, 2020

Choose a reason for hiding this comment

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

Is there any reason to call list_datasets at all if current_schema is set?

Edit: I see this was already-existing logic, just moved. Won't block the PR on this.

Copy link
Collaborator

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Lovely! Thank you for adding tests as well.

@tswast tswast merged commit 8c1450f into googleapis:master Nov 18, 2020
@romainr
Copy link
Contributor

romainr commented Nov 19, 2020

Thanks!

And for the record this is a backward incompatible change, so good to be explicit in the release notes

@tswast
Copy link
Collaborator

tswast commented Nov 19, 2020

@romainr Good point. I've updated the CHANGELOG and release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants