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

chore: make args required-keywords in make read_postgres, read_sqlite to be consistent with read_mysql #8712

Closed
ncclementi opened this issue Mar 20, 2024 · 5 comments · Fixed by #8829
Assignees
Labels
duckdb The DuckDB backend

Comments

@ncclementi
Copy link
Contributor

Recently, in #8656 we discussed making the arguments of read_mysql required-keywords, and agreed that as a followup we will make this consistent in read_postgresand read_sqlite.

Action Item:

  • Make args in read_postgresand read_sqlite required-keyword
@ncclementi ncclementi added the duckdb The DuckDB backend label Mar 20, 2024
@ncclementi ncclementi self-assigned this Mar 20, 2024
@ncclementi ncclementi moved this from backlog to todo in Ibis planning and roadmap Mar 20, 2024
gforsyth pushed a commit that referenced this issue Apr 2, 2024
…ite (#8829)

In #8656 we made the args for
`read_mysql` required-keyword.

This PR, makes the args for `read_postgres` and `read_sqlite` to be
required-keyword to be consistent.
It also fixes a broken test that runs only locally, `test_read_postgres`
(thanks @gforsyth for the patch suggestion).

- Closes #8712
@iangow
Copy link

iangow commented May 6, 2024

Not clear that this "consistency" is helpful, as it introduces inconsistencies elsewhere. If you search here, you will see many references to schema = . So sometimes for PostgreSQL users, it will be schema = and other times database =. (One can have multiple databases, each with multiple schemas in PostgreSQL.)

@gforsyth
Copy link
Member

gforsyth commented May 6, 2024

Hey @iangow -- hopefully on that page any reference to schema= will be exclusively related to an Ibis schema mapping column names to dtypes.

If there are any remaining references to schema as a hierarchical location mapping, those should definitely be removed.

@iangow
Copy link

iangow commented May 6, 2024

Hey @iangow -- hopefully on that page any reference to schema= will be exclusively related to an Ibis schema mapping column names to dtypes.

If there are any remaining references to schema as a hierarchical location mapping, those should definitely be removed.

It seems there are several places where this is not the case. Sometimes this argument is listed as [deprecated], but not always (e.g., when there's no documentation). For read_postgres() with the DuckDB backend, there was (it seems) no deprecation … the argument just disappeared.

Not sure that every PostgreSQL user (read_postgres()) will know that "database" is being used in the MySQL sense without more documentation.

@iangow
Copy link

iangow commented May 6, 2024

This note appears at times:

::: {.callout-note}
Ibis does not use the word schema to refer to database hierarchy. A collection of tables is referred to as a database. A collection of database is referred to as a catalog. These terms are mapped onto the corresponding features in each backend (where available), regardless of whether the backend itself uses the same terminology.
:::

It doesn't render properly in most cases. It looks good under list_databases(), but not under insert() or list_tables() or truncate_table(). Perhaps this note should also go in read_postgres().

@gforsyth
Copy link
Member

gforsyth commented May 6, 2024

If an API is marked experimental we may have just removed it outright -- I'll look at the docs rendering issues and also add a note to read_postgres

cpcloud pushed a commit that referenced this issue May 7, 2024
…9133)

Callouts inside of a parameter description aren't being rendered in the
docs, so I've moved this callout into the main body of the docstring so
it renders correctly.

I've also added the same callout to the `read_postgres` method on the
DuckDB backend, as postgres users may try to use `schema` in the
hierarchical sense.

xref #8712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duckdb The DuckDB backend
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants