fix(sqlalchemy-bigquery): update literal binds test for SQLAlchemy 2.0#17029
Conversation
Removes the skip for SQLAlchemy >= 2.0 on test_compiled_query_literal_binds. Modifies the test to wrap the compiled query in sqlalchemy.text(str(compiled)) before execution, as passing a Compiled object directly to Connection.execute() is not supported in SQLAlchemy 2.0. Fixes sqlalchemy-bigquery portion of Issue #16042.
There was a problem hiding this comment.
Code Review
This pull request enables the test_compiled_query_literal_binds test for SQLAlchemy 2.0 and updates the execution logic to use sqlalchemy.text(str(compiled)). Feedback suggests that using sqlalchemy.text() may cause issues if the SQL contains colons, as SQLAlchemy might misinterpret them as bind parameters. It is recommended to use exec_driver_sql where available to ensure safer execution and better compatibility across different SQLAlchemy versions.
| compiled = q.compile(engine, compile_kwargs={"literal_binds": True}) | ||
| with engine.connect() as conn: | ||
| result = conn.execute(compiled).fetchall() | ||
| result = conn.execute(sqlalchemy.text(str(compiled))).fetchall() |
There was a problem hiding this comment.
Using sqlalchemy.text() on a string generated with literal_binds=True can be problematic if the SQL contains colons (e.g., in timestamps or string literals), as SQLAlchemy will attempt to interpret them as bind parameters. A more robust approach that maintains compatibility across SQLAlchemy 1.3, 1.4, and 2.0 is to use exec_driver_sql when available, as it bypasses parameter parsing entirely and is the recommended way to execute raw SQL strings in modern SQLAlchemy.
| result = conn.execute(sqlalchemy.text(str(compiled))).fetchall() | |
| if hasattr(conn, "exec_driver_sql"): | |
| result = conn.exec_driver_sql(str(compiled)).fetchall() | |
| else: | |
| result = conn.execute(compiled).fetchall() |
| ) | ||
| with engine_using_test_dataset.connect() as conn: | ||
| result = conn.execute(compiled).fetchall() | ||
| result = conn.execute(sqlalchemy.text(str(compiled))).fetchall() |
There was a problem hiding this comment.
Same as above: using exec_driver_sql is safer for literal binds to avoid potential issues with colon parsing in SQLAlchemy 1.4 and 2.0.
| result = conn.execute(sqlalchemy.text(str(compiled))).fetchall() | |
| if hasattr(conn, "exec_driver_sql"): | |
| result = conn.exec_driver_sql(str(compiled)).fetchall() | |
| else: | |
| result = conn.execute(compiled).fetchall() |
This PR fixes the sqlalchemy-bigquery portion of Issue #16042.
Problem:
test_compiled_query_literal_bindswas skipped for SQLAlchemy >= 2.0 because it was failing. The failure occurs because in SQLAlchemy 2.0,Connection.execute()expects an executable object or a string (wrapped intext()), and passing aCompiledobject directly is no longer supported.Fix:
sqlalchemy.text()before passing it toconn.execute().This allows the test to run and pass on SQLAlchemy 2.0.