hy144328 commented on code in PR #42803:
URL: https://github.com/apache/superset/pull/42803#discussion_r3737994925
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -286,7 +286,7 @@ def test_dml(
"(shillelagh.exceptions.ProgrammingError) DML not enabled in database "
'"database2"\n[SQL: INSERT INTO "database2.table2" (a, b) '
"VALUES (3, 'thirty')]\n(Background on this error at: "
- "https://sqlalche.me/e/14/f405)"
Review Comment:
Perhaps use a regular expresssion here.
SQLAlchemy 2.1 is not far into the future, either.
##########
tests/integration_tests/db_engine_specs/hive_tests.py:
##########
@@ -357,7 +357,9 @@ def test_where_latest_partition(mock_method):
columns,
)
query_result = str(result.compile(compile_kwargs={"literal_binds": True}))
- assert "SELECT \nWHERE ds = '01-01-19' AND hour = 1" == query_result
Review Comment:
Perhaps it is worthwhile using an SQL prettifier here to make the test more
robust unless you actually care for character-by-character equality.
##########
tests/integration_tests/superset_test_config.py:
##########
@@ -58,6 +59,23 @@
"SQLite Database support for metadata databases will be "
"removed in a future version of Superset."
)
+ # SQLAlchemy 2.0 changed the default poolclass for file-based SQLite
Review Comment:
Will there still be SQLite support in Superset 7?
If yes, I might have missed it, but I did not see a similar update in the
production code, in addition to the test here.
Or is the responsibility delegated from a default to the admin?
##########
superset/databases/utils.py:
##########
@@ -72,17 +73,37 @@ def get_table_metadata(database: Any, table: Table) ->
TableMetadataResponse:
:return: Dict table metadata ready for API response
"""
keys = []
- columns = database.get_columns(table)
- primary_key = database.get_pk_constraint(table)
+ table_missing = False
+ try:
+ columns = database.get_columns(table)
+ primary_key = database.get_pk_constraint(table)
+ foreign_keys = get_foreign_keys_metadata(database, table)
+ indexes = get_indexes_metadata(database, table)
+ table_comment = database.get_table_comment(table)
+ except NoSuchTableError:
+ # SQLAlchemy 2.0's sqlite dialect raises NoSuchTableError from
Review Comment:
Sounds like SQLAlchemy 2 made the behavior for SQLite more consistent to the
other databases.
Would it make sense to seize the opportunity of a Superset breaking change
to remove the SQLite extra treatment in test as well as API?
##########
superset/utils/core.py:
##########
@@ -815,6 +815,7 @@ def ping_connection(connection: Connection, branch: bool)
-> None:
# the SELECT of a scalar value without a table is
# appropriately formatted for the backend
connection.scalar(select(1))
+ connection.rollback() # pylint: disable=consider-using-transaction
Review Comment:
Out of curiosity, as mentioned, my unit tests all failed because during
Superset initialization it was not possible to open a second transaction.
Is this the fix for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
