BUG: Fixed ADBC to_sql creation of table when using public schema #57974

shabab477 · 2024-03-23T16:19:24Z

Problem: table on public schema being lost when tried to be created

Solution: used db_schema_name argument to specify schema name of adbc_ingest

closes BUG: ADBC Postgres writer incorrectly names the table #57539 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

jorisvandenbossche

Thanks for the PR!

jorisvandenbossche · 2024-03-25T11:54:23Z

pandas/tests/io/test_sql.py

+@pytest.mark.parametrize("conn", postgresql_connectable)
+def test_to_sql_on_public_schema_postgres(conn, request):


Not super familiar with this, but is this specific to Postgres? (do we have other tested database flavors that support schema's?)

It seems that the existing test for the schema keyword, test_psycopg2_schema_support is indeed postgres specific, but is also only run for the psycopg2 driver. Could we extend that existing test to run with all postgresql_connectable ?

Actually, it seems that postgresql_connectable doesn't include an ADBC-based connection, so to test ADBC you will have to test it with postgresql_adbc_conn

(do we have other tested database flavors that support schema's?)

AFAIK sqlite is the only database that doesn't have schemas. So actually can use the all_connectable fixture and imperatively xfail when "sqlite" in conn_name

yes, it seems the issue was indeed postgres specific. MySQL seems to have a different concept of schemas, as seen here.

You are right about postgresql_connectable not having adbc based connections. What I did instead, just used all_connectable and tested for postgresql specific connectables.

WillAyd · 2024-03-25T12:58:25Z

Very nice PR. Can you also add a whatsnew note for version 2.2.2? Outside of that and @jorisvandenbossche comment this lgtm

WillAyd · 2024-03-25T21:47:22Z

pandas/tests/io/test_sql.py

+    df_out = sql.read_sql_table("test_public_schema", conn, schema="public")
+    assert test_data.equals(df_out)
+
+    # Cleanup


The fixture is supposed to take care of the cleanup for you - are you not seeing that?

No I wasn't seeing that, unfortunately I have no experience with pytest. Took this issue as a learning experience and did get to learn a lot. Found out how its working. Removed this cleanup part but do look at my other reply please.

Cool no worries. I just wanted to double check as a reviewer - its hard to see the issues that lead developers down certains paths at times.

Took this issue as a learning experience and did get to learn a lot.

That's great and we appreciate the contribution

WillAyd · 2024-03-25T21:47:52Z

doc/source/whatsnew/v2.2.2.rst

@@ -24,6 +24,7 @@ Bug fixes
 ~~~~~~~~~
 - :meth:`DataFrame.__dataframe__` was showing bytemask instead of bitmask for ``'string[pyarrow]'`` validity buffer (:issue:`57762`)
 - :meth:`DataFrame.__dataframe__` was showing non-null validity buffer (instead of ``None``) ``'string[pyarrow]'`` without missing values (:issue:`57761`)
+- :meth:`DataFrame.to_sql` was failing to find the right table on PostgreSQL when using the ``public`` schema (:issue:`57539`)


I think this technically would have affected any database and any schema, so we don't need to be overly specific about it being Postgres with the public schema

WillAyd · 2024-03-25T21:50:35Z

pandas/tests/io/test_sql.py

+
+    conn = request.getfixturevalue(conn)
+
+    from sqlalchemy.engine import Engine


Are you sure you need to explicitly create the schema / table this way? I am under the impression that the to_sql call should take care of that anyway.

The reason I am somewhat against doing it via sqlalchemy is that ADBC connections do not use / required sqlalchemy at all. So this starts to intertwine the dependencies of those libraries in ways that is unnecessary. For example, if you look at the ADBC fixtures they do not use sqlalchemy to create / drop tables

Didn't know to_sql also creates the table but apparently it does. However, I do see an issue now in the failing pipeline: data = b"\xff\x19\x04#42000Unknown database 'public'" and this happens for mysql_pymysql_engine . Maybe I can add a statement CREATE DATABASE some_db; and execute it with sqlalchemy and clean up at the end? wdyt?

You shouldn't need to manually construct the CREATE DATABASE statement. It looks like I was wrong before about which databases have schemas versus those that do not, and apparently MySQL does not have the same concept of what a schema is as postgresql.

https://stackoverflow.com/a/11618350/621736

So I think you can go back to @jorisvandenbossche original suggestion and just use the postgresql_adbc_conn fixture instead of the all fixture

WillAyd

Very minor comment otherwise lgtm. @jorisvandenbossche anything else from your end?

WillAyd · 2024-03-27T17:39:29Z

pandas/tests/io/test_sql.py

@@ -1373,6 +1373,30 @@ def insert_on_conflict(table, conn, keys, data_iter):
        pandasSQL.drop_table("test_insert_conflict")


+@pytest.mark.parametrize("conn", all_connectable)
+def test_to_sql_on_public_schema(conn, request):
+    if "postgresql" not in conn:


nit but I think this can be if "sqlite" in conn or "mysql" in conn: - that way we exclude those two databases but in theory any new one beyond this that gets tested (which more than likely has a schema) will run here

Right, makes more sense and safe. Done

mroeschke · 2024-03-27T17:49:54Z

pandas/tests/io/test_sql.py

+    )
+
+    df_out = sql.read_sql_table("test_public_schema", conn, schema="public")
+    assert test_data.equals(df_out)


Suggested change

assert test_data.equals(df_out)

tm.assert_frame.equal(test_data, df_out)

Thanks ! Done

pandas/tests/io/test_sql.py

Problem: - Table on public schema being lost when tried to be created Solution: - Used db_schema_name argument to specify schema name of adbc_ingest

mroeschke · 2024-03-28T17:56:03Z

Thanks @shabab477

…e when using public schema

…f table when using public schema) (#58050) Backport PR #57974: BUG: Fixed ADBC to_sql creation of table when using public schema Co-authored-by: Shabab Karim <[email protected]>

…ndas-dev#57974) 57539: Fixed creation unnamed table when using public schema Problem: - Table on public schema being lost when tried to be created Solution: - Used db_schema_name argument to specify schema name of adbc_ingest

shabab477 force-pushed the bugfix/57539-fix-to-sql branch 3 times, most recently from 1e206b0 to 9b7c3e5 Compare March 24, 2024 11:21

jorisvandenbossche added Bug IO SQL to_sql, read_sql, read_sql_query Arrow pyarrow functionality Sprints Sprint Pull Requests labels Mar 25, 2024

jorisvandenbossche changed the title ~~57539: Fixed creation unnamed table when using public schema~~ BUG: Fixed ADBC to_sql creation of table when using public schema Mar 25, 2024

jorisvandenbossche requested a review from WillAyd March 25, 2024 11:45

jorisvandenbossche reviewed Mar 25, 2024

View reviewed changes

shabab477 force-pushed the bugfix/57539-fix-to-sql branch 3 times, most recently from 8daa913 to 8ddfd68 Compare March 25, 2024 21:44

WillAyd requested changes Mar 25, 2024

View reviewed changes

shabab477 force-pushed the bugfix/57539-fix-to-sql branch 5 times, most recently from 6bb2eb8 to b45a340 Compare March 27, 2024 08:17

WillAyd reviewed Mar 27, 2024

View reviewed changes

WillAyd added this to the 2.2.2 milestone Mar 27, 2024

mroeschke reviewed Mar 27, 2024

View reviewed changes

shabab477 force-pushed the bugfix/57539-fix-to-sql branch from b45a340 to 0fdf1a7 Compare March 27, 2024 19:29

shabab477 requested review from WillAyd, mroeschke and jorisvandenbossche March 27, 2024 19:31

WillAyd reviewed Mar 27, 2024

View reviewed changes

pandas/tests/io/test_sql.py Outdated Show resolved Hide resolved

shabab477 force-pushed the bugfix/57539-fix-to-sql branch from e88a9cb to 9efb646 Compare March 27, 2024 23:19

57539: Fixed creation unnamed table when using public schema

6afa1b2

Problem: - Table on public schema being lost when tried to be created Solution: - Used db_schema_name argument to specify schema name of adbc_ingest

shabab477 force-pushed the bugfix/57539-fix-to-sql branch from 9efb646 to 6afa1b2 Compare March 27, 2024 23:22

WillAyd approved these changes Mar 28, 2024

View reviewed changes

mroeschke approved these changes Mar 28, 2024

View reviewed changes

mroeschke merged commit aba759d into pandas-dev:main Mar 28, 2024
46 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Mar 28, 2024

Backport PR pandas-dev#57974: BUG: Fixed ADBC to_sql creation of tabl…

5eecbd0

…e when using public schema

meeseeksmachine mentioned this pull request Mar 28, 2024

Backport PR #57974 on branch 2.2.x (BUG: Fixed ADBC to_sql creation of table when using public schema) #58050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fixed ADBC to_sql creation of table when using public schema #57974

BUG: Fixed ADBC to_sql creation of table when using public schema #57974

shabab477 commented Mar 23, 2024 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche Mar 25, 2024

jorisvandenbossche Mar 25, 2024

WillAyd Mar 25, 2024

shabab477 Mar 27, 2024

WillAyd commented Mar 25, 2024

WillAyd Mar 25, 2024

shabab477 Mar 26, 2024

WillAyd Mar 26, 2024

WillAyd Mar 25, 2024

shabab477 Mar 26, 2024

WillAyd Mar 25, 2024

shabab477 Mar 26, 2024

WillAyd Mar 26, 2024

WillAyd left a comment

WillAyd Mar 27, 2024

shabab477 Mar 28, 2024

mroeschke Mar 27, 2024

shabab477 Mar 27, 2024

mroeschke commented Mar 28, 2024

		@pytest.mark.parametrize("conn", postgresql_connectable)
		def test_to_sql_on_public_schema_postgres(conn, request):


		conn = request.getfixturevalue(conn)

		from sqlalchemy.engine import Engine

	assert test_data.equals(df_out)
	tm.assert_frame.equal(test_data, df_out)

BUG: Fixed ADBC to_sql creation of table when using public schema #57974

BUG: Fixed ADBC to_sql creation of table when using public schema #57974

Conversation

shabab477 commented Mar 23, 2024 • edited Loading

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke commented Mar 28, 2024

shabab477 commented Mar 23, 2024 •

edited

Loading