betodealmeida commented on code in PR #37311: URL: https://github.com/apache/superset/pull/37311#discussion_r2714355565
########## tests/unit_tests/commands/importers/v1/examples_test.py: ########## @@ -0,0 +1,140 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for the examples importer, specifically SQL transpilation.""" + +from unittest.mock import MagicMock, patch + +from superset.commands.importers.v1.examples import transpile_virtual_dataset_sql + + +def test_transpile_virtual_dataset_sql_no_sql(): + """Test that configs without SQL are unchanged.""" + config = {"table_name": "my_table", "sql": None} + transpile_virtual_dataset_sql(config, 1) + assert config["sql"] is None + + +def test_transpile_virtual_dataset_sql_empty_sql(): + """Test that configs with empty SQL are unchanged.""" + config = {"table_name": "my_table", "sql": ""} + transpile_virtual_dataset_sql(config, 1) + assert config["sql"] == "" + + +@patch("superset.commands.importers.v1.examples.db") +def test_transpile_virtual_dataset_sql_database_not_found(mock_db): + """Test graceful handling when database is not found.""" + mock_db.session.query.return_value.get.return_value = None + + config = {"table_name": "my_table", "sql": "SELECT * FROM foo"} + original_sql = config["sql"] + + transpile_virtual_dataset_sql(config, 999) + + # SQL should remain unchanged + assert config["sql"] == original_sql + + +@patch("superset.commands.importers.v1.examples.db") +@patch("superset.commands.importers.v1.examples.transpile_to_dialect") +def test_transpile_virtual_dataset_sql_success(mock_transpile, mock_db): + """Test successful SQL transpilation.""" + # Setup mock database + mock_database = MagicMock() + mock_database.db_engine_spec.engine = "mysql" + mock_db.session.query.return_value.get.return_value = mock_database + + # Setup mock transpilation + mock_transpile.return_value = "SELECT * FROM `foo`" + + config = {"table_name": "my_table", "sql": "SELECT * FROM foo"} + transpile_virtual_dataset_sql(config, 1) + + # SQL should be transpiled + assert config["sql"] == "SELECT * FROM `foo`" + mock_transpile.assert_called_once_with("SELECT * FROM foo", "mysql") + + +@patch("superset.commands.importers.v1.examples.db") +@patch("superset.commands.importers.v1.examples.transpile_to_dialect") +def test_transpile_virtual_dataset_sql_no_change(mock_transpile, mock_db): + """Test when transpilation returns same SQL (no dialect differences).""" + mock_database = MagicMock() + mock_database.db_engine_spec.engine = "postgresql" + mock_db.session.query.return_value.get.return_value = mock_database + + original_sql = "SELECT * FROM foo" + mock_transpile.return_value = original_sql + + config = {"table_name": "my_table", "sql": original_sql} + transpile_virtual_dataset_sql(config, 1) + + assert config["sql"] == original_sql + + +@patch("superset.commands.importers.v1.examples.db") +@patch("superset.commands.importers.v1.examples.transpile_to_dialect") +def test_transpile_virtual_dataset_sql_error_fallback(mock_transpile, mock_db): + """Test graceful fallback when transpilation fails.""" + from superset.exceptions import QueryClauseValidationException + + mock_database = MagicMock() + mock_database.db_engine_spec.engine = "mysql" + mock_db.session.query.return_value.get.return_value = mock_database + + # Simulate transpilation failure + mock_transpile.side_effect = QueryClauseValidationException("Parse error") + + original_sql = "SELECT SOME_POSTGRES_SPECIFIC_FUNCTION() FROM foo" + config = {"table_name": "my_table", "sql": original_sql} + + # Should not raise, should keep original SQL + transpile_virtual_dataset_sql(config, 1) + assert config["sql"] == original_sql + + +@patch("superset.commands.importers.v1.examples.db") +@patch("superset.commands.importers.v1.examples.transpile_to_dialect") +def test_transpile_virtual_dataset_sql_complex_query(mock_transpile, mock_db): + """Test transpilation of a more complex SQL query.""" + mock_database = MagicMock() + mock_database.db_engine_spec.engine = "duckdb" + mock_db.session.query.return_value.get.return_value = mock_database + + original_sql = """ + SELECT + DATE_TRUNC('month', created_at) as month, + COUNT(*) as count + FROM orders + WHERE status = 'completed' + GROUP BY 1 + """ + transpiled_sql = """ + SELECT + DATE_TRUNC('month', created_at) AS month, Review Comment: This one is easy. Let's add tests transpiling to a bigger variety of DBs -- at least something that doesn't have `DATE_TRUNC`, like Clickhouse (I think?). ########## superset/commands/importers/v1/examples.py: ########## @@ -41,9 +43,55 @@ from superset.dashboards.schemas import ImportV1DashboardSchema from superset.databases.schemas import ImportV1DatabaseSchema from superset.datasets.schemas import ImportV1DatasetSchema +from superset.exceptions import QueryClauseValidationException +from superset.models.core import Database +from superset.sql.parse import transpile_to_dialect from superset.utils.core import get_example_default_schema from superset.utils.decorators import transaction +logger = logging.getLogger(__name__) + + +def transpile_virtual_dataset_sql(config: dict[str, Any], database_id: int) -> None: + """ + Transpile virtual dataset SQL to the target database dialect. + + This ensures that virtual datasets exported from one database type + (e.g., PostgreSQL) can be loaded into a different database type + (e.g., MySQL, DuckDB, SQLite). + + Args: + config: Dataset configuration dict (modified in place) + database_id: ID of the target database + """ + sql = config.get("sql") + if not sql: + return + + database = db.session.query(Database).get(database_id) + if not database: + logger.warning("Database %s not found, skipping SQL transpilation", database_id) + return + + target_engine = database.db_engine_spec.engine + try: + transpiled_sql = transpile_to_dialect(sql, target_engine) Review Comment: This only works when `sql` is in the "Generic" dialect, but here it's a specific dialect coming from the YAML file. A good solution would be to modify the `transpile_to_dialect` function to receive a source dialect, defaulting to the generic `Dialect`. -- 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]
