-
Notifications
You must be signed in to change notification settings - Fork 603
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
feat: Ability to Distinguish between Table and Views #8382
Comments
This is an exceptionally hairy problem for something that seems so simple. Current APIThe goal of the existing API is to treat everything tabular as an abstract table, so tables, views, materialized views, etc, are all "tables" in this context. So you If we separate into
|
I think the suggestion of separating out backend terminology (where "Table" is different from "view", "materialized view", ...) from ibis terminology (where everything tabular is an
|
Sure, but the people that are not familiarized with SQL won't be looking at "views" for example, they'll treat everything as a table. I'm not sure about the name either, but the idea is to just be able to list the
What I understood was that we will only have
I think we only want to move/create |
I think that'd be even more confusing. That would mean that users wanting to see only the physical tables in a database (perhaps to enumerate and drop them all), they'd need to do something like: actual_tables = set(con.list_tables(database="my-database")).difference(con.ddl.list_views(database="my-database")) If we're going to add an accessor that lets you explicitly list out each of the tabular things in a backend, I think we'll want a way to do this for all the different kinds of things, not everything except tables. |
An alternate proposal:
This keeps everything at the top level (nice for discoverability, and no need to name a (to be clear, I'm just talking through issues I see with the |
(in reference to the previous to last comment )If that's that case then I think the accessor complicate things more for the user. The current open PR separates physical tables from views, without an accessor. But it sounds like the name |
Agreed that it's a rough edge, but I think of this as a thing more advanced users might want, and they will 🤞 read docs to find out how to do Thing™
It might be confusing for users, but I think it's an acceptable trade-off. I don't want to expose new users to needing to think about engine-specific details.
I think that's why
Yeah, I think that's fine - it would clean up the top-level namespace. I do not like |
That's fair. Could we deprecate
If we go the route of deprecating |
I think I like this. It'll be a big change for long-time users, but I think a shorter method (property) name is nice. To summarize the target state:
|
Yay, I agree this is a good end state. One small addition, I think |
I thought it was a mapping already, so can't you call |
You can, but I don't see a reason not to make it callable as well to mirror the existing |
I guess I'm maybe ... -0.25 on that only because we'd then have at least 3 ways to get the list of tables:
seems like the latter two are probably sufficient. |
well, in the long run |
Based on what's discussed here and after chatting with @gforsyth End Goal
|
Given that we are planning on having a create temporary view, seems appropriate to tackle this one first #8727 before continuing with the accessor. |
Is your feature request related to a problem?
I see that we have an option to find Tables using
con.list_tables()
method, but in many DBs Views are also considered as tables, as in INFORMATION_SCHEMA most DB's list Views as wellIf there is a way to add a param to backend to remove Views that would be great
Describe the solution you'd like
Add a default param
table_types=[]
tocon.list_tables()
FunctionIn all DB's logic add a condition,
if table_types:
table_type IN ( ','.join(table_type) )
table_type IN ( ','.join(table_type) )
table_type IN ( ','.join(table_type) )
etc
What version of ibis are you running?
8.0.0
What backend(s) are you using, if any?
SQL Server
Code of Conduct
The text was updated successfully, but these errors were encountered: