Kunal8954 commented on code in PR #43808:
URL: https://github.com/apache/superset/pull/43808#discussion_r3939125490
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -773,6 +773,97 @@ def test_import_dataset_no_folder(mocker: MockerFixture,
session: Session) -> No
assert sqla_table.folders is None
+def test_import_dataset_skips_has_table_check_without_data_uri(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Importing a dataset with no ``data`` URI should never call
+ ``Database.has_table`` - its result only ever gates ``load_data``, which
+ is already a no-op when there's no data URI to load. Skipping the call
+ avoids an unnecessary round trip to the target database on every
+ imported dataset, which is what made bulk imports of many datasets slow
+ enough to hit the gunicorn worker timeout.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ has_table = mocker.patch.object(Database, "has_table")
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = {
+ "table_name": "no_data_table",
+ "main_dttm_col": None,
+ "description": None,
+ "default_endpoint": None,
+ "offset": 0,
+ "cache_timeout": None,
+ "schema": None,
+ "sql": None,
+ "params": None,
+ "template_params": None,
+ "filter_select_enabled": False,
+ "fetch_values_predicate": None,
+ "extra": None,
+ "uuid": uuid.uuid4(),
+ "metrics": [],
+ "columns": [],
+ "database_uuid": database.uuid,
+ "database_id": database.id,
+ }
+
+ import_dataset(config)
+
+ has_table.assert_not_called()
+
+
+def test_import_dataset_checks_has_table_with_data_uri(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ When a ``data`` URI is present, ``Database.has_table`` should still be
+ consulted to decide whether the data needs to be (re-)loaded.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ has_table = mocker.patch.object(Database, "has_table", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = {
+ "table_name": "has_data_table",
+ "main_dttm_col": None,
+ "description": None,
+ "default_endpoint": None,
+ "offset": 0,
+ "cache_timeout": None,
+ "schema": None,
+ "sql": None,
+ "params": None,
+ "template_params": None,
+ "filter_select_enabled": False,
+ "fetch_values_predicate": None,
+ "extra": None,
+ "uuid": uuid.uuid4(),
+ "metrics": [],
+ "columns": [],
+ "database_uuid": database.uuid,
+ "database_id": database.id,
+ "data": "https://example.com/data.csv";,
+ }
+
+ import_dataset(config)
+
+ has_table.assert_called_once()
Review Comment:
Good point, the has_table=True case never reached load_data so it proved
very little. Patched load_data in both tests now: not called when the table
exists, called once when it doesn't. Checked it catches the inverted
condition you described — flipping `not table_exists` fails both new
assertions.
--
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]
