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

refactor(api): ddl accessor #9832

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ncclementi
Copy link
Contributor

@ncclementi ncclementi commented Aug 13, 2024

Very rough start, so far I created the DDL accessor and I'm trying to get to run the some methods as they are (no changes yet to functionality) from the accessor like con.ddl.list_, this works. But I'm having problems triggering the deprecation when doing con.list_.

I thought if I deprecate the abstract methods that would do it, but it doesn't because it looks immediately for the backend specific implementation. Not sure what's the best solution here design wise.

** How do we deprecate in this case? **
If I move the deprecation to the Backend implementation of let's say list_tables as an example, it will trigger both when doing con.list_tables() and con.ddl.list_tables()

One option would be taking the methods out of the backend class, to a let's say DDL and access them via the accessor, and when the ones on the Backend class get accessed con.list_ they'll warn and point to the con.ddl.list_. If we do this, I wonder if having a DDLBase with all abstract methods might make sense here, something similar to what we do with the BaseBackend one.

EDIT:
After discussing with Gil here are the steps to proceed:

  • remove ddl abstract methods in BaseBackend (like list_tables)
  • deprecate list_* in Backend specific
  • create _list_* in Backend specific
  • create list_* in the ddl accessor, point it to the self._backend._list_* method

TODO:

  • Get all the list_* related work, working for duckdb
  • Move to other backends
  • Leave other ddl operations to followup PR for simplicity.

cc: @gforsyth for when you are back

Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@ncclementi ncclementi force-pushed the ddl_accessor branch 2 times, most recently from 71b1c26 to e0503f8 Compare August 19, 2024 21:31
@ncclementi
Copy link
Contributor Author

@gforsyth I did a first path at

  • Implementing the list_* for duckdb
  • Safeguarding the DDLAccessor with a higher level of NotImplementedError.
  • Modifying con.tables to use list_* making sure that it uses the one that are implemented for the specific backend.
  • Added api tests for all the list_* and one for modified the previous con.list_tables test (now named test_list_all_tables_and_views)

Right now we have a lot of tests that rely on asserts that use con.list_tables I haven't modified any of them yet since in some cases it should be con.tables and others might be con.ddl.list_tables. If we are good with this idea/design, I'll move forward with cleaning up tests and start working on the other backends.

ibis/backends/__init__.py Outdated Show resolved Hide resolved
ibis/backends/duckdb/__init__.py Outdated Show resolved Hide resolved
ibis/backends/duckdb/__init__.py Outdated Show resolved Hide resolved
@ncclementi ncclementi changed the title refactor: DDL accessor [WIP] refactor(api): DDL accessor Aug 23, 2024
@ncclementi
Copy link
Contributor Author

I'm trying to get the DuckDB backend green so I can move on to the other backend's implementation but I'm having a few issues that I idk where they are coming from. I think if we can fix those we are on a safe point to move onto the other backends.

…se` in `_fetch_from_cursor` (ibis-project#9913)

Previously we were trying to close a cursor after an exception was
raised in `raw_sql`, which already closes the cursor in the case of an
exception. This is not allowed by the oracledb driver, so just close the
cursor on success.
@ncclementi ncclementi changed the title refactor(api): DDL accessor refactor(api): ddl accessor Aug 26, 2024
@ncclementi
Copy link
Contributor Author

Discussion for postgres:

So, we are currently on main doing something weird with list_tables() in postgres that doesn't match necessarily any behavior in psotgres sql.

I created a table, a temp table, a view a temp view and a foreign table in psql.

  • \dt only lists table and temp table
  • \dv only lists view and temp view
  • \det only list foreign table
  • \dvtE list them all

on Ibis con.list_tables()

  • lists the view, table, and foreign table (no temp table or temp view)

On the PR I propose,

  • having the same than in duckdb case list_tables, list_temp_tables, list_views, list_temp_views
  • add list_foreign_tables
  • con.tables: list them all

@gforsyth @cpcloud does this sound reasonable?

@gforsyth
Copy link
Member

does this sound reasonable?

yeah, I think that sounds good!

@github-actions github-actions bot added tests Issues or PRs related to tests postgres The PostgreSQL backend sqlite The SQLite backend labels Sep 20, 2024
@github-actions github-actions bot added mysql The MySQL backend datafusion The Apache DataFusion backend duckdb The DuckDB backend labels Sep 20, 2024
@github-actions github-actions bot added the clickhouse The ClickHouse backend label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clickhouse The ClickHouse backend datafusion The Apache DataFusion backend duckdb The DuckDB backend mysql The MySQL backend postgres The PostgreSQL backend sqlite The SQLite backend tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Ability to Distinguish between Table and Views
3 participants