You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "drivaspreset (via GitHub)" <gi...@apache.org> on 2026/08/27 18:14:50 UTC
[PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
drivaspreset opened a new pull request, #43607:
URL: https://github.com/apache/superset/pull/43607
### SUMMARY
Closes the unit-test gaps in `PrestoEngineSpec`. **No production code changes** — this is tests
only
Most of this file's behaviour was either proven only in the integration suite (which needs an app
and a metadata DB) or not proven at all. Several methods were completely uncovered: `handle_cursor`,
`get_tracking_url`, `get_function_names`, `get_allow_cost_estimate`, `get_catalog_names`, and the
`PrestoBaseEngineSpec.convert_dttm` override. That matters because the logic here is pure and
deterministic — reproducing its failures end-to-end needs a live Presto cluster, but pinning it down
needs only mocks.
`superset/db_engine_specs/presto.py`: **306 → 161 missed statements (34% → 63%)**, and
**+86 test cases** (`tests/unit_tests/db_engine_specs/test_presto.py` 47 → 131,
`tests/unit_tests/models/core_test.py` 77 → 79).
The commits are grouped by behaviour area and are readable in order:
| Commit | Area | Cases |
|---|---|---:|
| 1 | error regexes and `_extract_error_message` | 19 |
| 2 | `convert_dttm` precision and `column_type_mappings` | 17 |
| 3 | `handle_cursor` and `get_tracking_url` | 9 |
| 4 | cost estimation | 13 |
| 5 | view, table and catalog discovery | 18 |
| 6 | `get_function_names` | 4 |
| 7 | `expand_data` | 4 |
| 8 | `Database.function_names`, null cost estimate | 3 |
#### Highlights
**All nine `custom_errors` regexes are now asserted.** Previously only the HTTP 401 pattern had a
test, so a typo in any of the other eight shipped silently — invisible until a user hit that exact
error. This is the most directly user-facing logic in the file: the difference between
*"Either the username "bob" or the password is incorrect"* and a raw pyhive string in a red toast.
**The Stop button.** `handle_cursor` is what notices a user clicked Stop — the click only updates the
query's status in the metadata database, and the `cursor.cancel()` this loop issues is what actually
releases the Presto cluster resources. That branch had no test; had it regressed, Stop would appear
to work in the UI while the query kept running.
**The millisecond/microsecond override.** `PrestoEngineSpec.convert_dttm` deliberately overrides
`PrestoBaseEngineSpec.convert_dttm` to render TIMESTAMP literals with millisecond rather than
microsecond precision, and nothing pinned that. It is also this file's only silent-failure mode: a
wrong-precision literal produces a *valid* query returning the wrong rows, with no error. A refactor
collapsing the two methods now fails a test instead of quietly shifting every Presto timestamp
filter.
**Schema round-trip.** `adjust_engine_params` percent-quotes the schema into the URI path with
`safe=""` and `get_schema_from_engine_params` unquotes it. Any asymmetry between the pair silently
points SQL Lab at the wrong schema, and a schema containing `/` would be read back as a different
name entirely. Tested as a round-trip over `/`, spaces, `%` and non-ASCII, since testing either half
alone cannot catch a mismatched pair.
#### Mirrored integration tests
Twelve cases mirror tests that already exist in
`tests/integration_tests/db_engine_specs/presto_tests.py`, reusing that suite's known-good
expectations. **The integration originals are intentionally left in place — that file is untouched by
this PR** (`git diff --stat` shows no changes to it). Each mirror's docstring names its original by
file and line, so the relationship is greppable rather than accidental. Removing the now-duplicated
integration tests, if ever wanted, is separate work.
#### Characterization tests — 8 tests that assert current, incorrect behaviour
These pass by pinning bugs, not by endorsing them. Each is labelled `CHARACTERIZATION test` in its
docstring and explains what a fix would change. No `xfail` anywhere — that would silently mask the
gap. Happy to split any of these out or drop them if maintainers would rather they land alongside
their fixes:
1. **`extract_errors` raises `KeyError` when `context` is omitted.** Four of the nine patterns
declare message placeholders their own regex never captures (`username`, `hostname`, `port`), so
the eager-`gettext` `str % dict` raises. Reachable from real call sites.
2. **`handle_cursor` raises `TypeError` on missing split counts.** `completedSplits`/`totalSplits`
are read with `stats.get(...)` and passed straight to `float()`, so a poll reporting a state
without split counts raises inside the SQL Lab worker rather than degrading to "no progress
information".
3. **`get_allow_cost_estimate` raises `InvalidVersion` on a malformed `version`.** An admin typo in
a database's Extra JSON raises rather than simply disabling cost estimation.
4. **`query_cost_formatter` raises `TypeError` on a null estimate value.** `humanize` guards
`int(value)` with `except ValueError` only, but a JSON `null` arrives as `None`. One null field
breaks the whole cost panel — note that an *absent* key is handled cleanly, so the asymmetry is
the bug.
5. **`expand_data` crashes on malformed JSON — two unguarded sites, one test each.** `destringify`
is `json.loads` with no `try/except` (`superset/result_set.py`), reached from both the ARRAY
branch and the ROW branch. Behind `PRESTO_EXPAND_DATA`, which defaults off. Both sites are
pinned, since a fix guarding only one would leave the other crashing.
6. **`get_function_names` raises `KeyError` on a DataFrame without a `Function` column.** Included
because "handles an empty function list" has two readings that behave differently; both are now
asserted explicitly.
7. **The "table does not exist" message double-quotes the table name.**
`TABLE_DOES_NOT_EXIST_REGEX` captures the surrounding single quotes, so users see
`The table "'default.foo'" does not exist.` Cosmetic, and changing user-facing copy is a product
decision.
#### Two other findings, not addressed here
- `tests/integration_tests/db_engine_specs/presto_tests.py::test_get_catalog_names` returns early
unless the example database is Presto, and asserts against a **list** while `get_catalog_names`
returns a **set** — it would fail if it ever ran. Left alone deliberately (that file is untouched);
the unit test added here is the method's first real coverage.
- `get_create_view` interpolates `schema` and `table` into `SHOW CREATE VIEW` with an f-string rather
than binding them. Those values come from the metadata database rather than user input, so this is
not a security finding under `SECURITY.md`; one test case pins the interpolation so the assumption
is visible if that ever changes.
Happy to open GitHub issues for any of the above that maintainers consider real — I did not want to
file eight issues speculatively.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes. The meaningful before/after is coverage of `superset/db_engine_specs/presto.py`:
```
BEFORE 508 statements 306 missed 180 branches 13 partial 34%
AFTER 508 statements 161 missed 180 branches 22 partial 63%
```
Methods that went from **entirely uncovered** to covered:
| Lines | Method |
|---|---|
| 308-315 | `PrestoBaseEngineSpec.convert_dttm` (microseconds) |
| 340-341 | `get_catalog_names` |
| 402-419 | `estimate_statement_cost` |
| 431-462 | `query_cost_formatter` |
| 1029-1030 | `get_allow_cost_estimate` |
| 1077 | `get_table_names` |
| 1105-1127 | `get_view_names` |
| 1235-1319 | `expand_data` |
| 1374-1385 | `get_create_view` |
| 1389-1393 | `get_tracking_url` |
| 1398-1442 | `handle_cursor` |
| 1451-1460 | `_extract_error_message` |
One caveat worth stating: **line coverage understates the change for the error regexes.** The loop in
`BaseEngineSpec.extract_errors` already read as covered after a single pattern matched, while 8 of 9
patterns had no assertion on their output at all. Read the case counts alongside the percentages.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/db_engine_specs/test_presto.py tests/unit_tests/models/core_test.py
```
210 tests, ~1.3s. No app, no database, no Presto cluster, no network — all mocked.
Coverage, for the numbers above:
```bash
python -m coverage run -m pytest tests/unit_tests/db_engine_specs/test_presto.py
python -m coverage report --include="*/db_engine_specs/presto.py" --show-missing
```
To confirm the integration suite is untouched:
```bash
git diff --stat master..HEAD -- tests/integration_tests/ # expect no output
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
None of the above applies: tests only
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43607:
URL: https://github.com/apache/superset/pull/43607#discussion_r3879400673
##########
tests/unit_tests/db_engine_specs/test_presto.py:
##########
@@ -412,6 +656,635 @@ def test_extract_errors_maps_401_to_access_denied() -> None:
assert result[0].error_type == SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR
+@pytest.mark.parametrize(
+ "raw_message,context,expected_error_type,expected_message",
+ [
+ pytest.param(
+ "line 1:8: Column 'bar' cannot be resolved",
+ {},
+ "COLUMN_DOES_NOT_EXIST_ERROR",
+ 'We can\'t seem to resolve the column "bar" at line 1:8.',
+ id="column_does_not_exist",
+ ),
+ pytest.param(
+ "Table 'default.foo' does not exist",
+ {},
+ "TABLE_DOES_NOT_EXIST_ERROR",
+ "The table \"'default.foo'\" does not exist. "
+ "A valid table must be used to run this query.",
+ id="table_does_not_exist",
+ ),
+ pytest.param(
+ "line 1:15: Schema 'bar' does not exist",
+ {},
+ "SCHEMA_DOES_NOT_EXIST_ERROR",
+ 'The schema "bar" does not exist. '
+ "A valid schema must be used to run this query.",
+ id="schema_does_not_exist",
+ ),
+ pytest.param(
+ "Access Denied: Invalid credentials",
+ {"username": "bob"},
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ 'Either the username "bob" or the password is incorrect.',
+ id="access_denied_invalid_credentials",
+ ),
+ pytest.param(
+ "presto error: Unexpected status code 401 b'Unauthorized'",
+ {},
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ "Unexpected HTTP 401 response. Check your credentials.",
+ id="access_denied_http_401",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 8] nodename nor "
+ "servname provided, or not known",
+ {"hostname": "badhost"},
+ "CONNECTION_INVALID_HOSTNAME_ERROR",
+ 'The hostname "badhost" cannot be resolved.',
+ id="invalid_hostname",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 60] Operation timed out",
+ {"hostname": "myhost", "port": 8080},
+ "CONNECTION_HOST_DOWN_ERROR",
+ 'The host "myhost" might be down, and can\'t be reached on port 8080.',
+ id="host_down_operation_timed_out",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 61] Connection refused",
+ {"hostname": "myhost", "port": 8080},
+ "CONNECTION_PORT_CLOSED_ERROR",
+ 'Port 8080 on hostname "myhost" refused the connection.',
+ id="port_closed",
+ ),
+ pytest.param(
+ "line 1:8: Catalog 'foo' does not exist",
+ {},
+ "CONNECTION_UNKNOWN_DATABASE_ERROR",
+ 'Unable to connect to catalog named "foo".',
+ id="unknown_catalog",
+ ),
+ ],
+)
+def test_extract_errors_matches_all_custom_error_patterns(
+ raw_message: str,
+ context: dict[str, Any],
+ expected_error_type: str,
+ expected_message: str,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import ErrorLevel, SupersetErrorType
+
+ result = PrestoEngineSpec.extract_errors(Exception(raw_message), context=context)
+
+ assert len(result) == 1
+ assert result[0].error_type == getattr(SupersetErrorType, expected_error_type)
+ assert result[0].message == expected_message
+ assert result[0].level == ErrorLevel.ERROR
+ assert result[0].extra is not None
+ assert result[0].extra["engine_name"] == "Presto"
+
+
+@pytest.mark.parametrize(
+ "raw_message,missing_placeholder",
+ [
+ pytest.param(
+ "Access Denied: Invalid credentials", "username", id="access_denied"
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 8] nodename nor "
+ "servname provided, or not known",
+ "hostname",
+ id="invalid_hostname",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 60] Operation timed out",
+ "hostname",
+ id="host_down",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 61] Connection refused",
+ "port",
+ id="port_closed",
+ ),
+ ],
+)
+def test_extract_errors_raises_key_error_without_context(
+ raw_message: str,
+ missing_placeholder: str,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(KeyError, match=missing_placeholder):
+ PrestoEngineSpec.extract_errors(Exception(raw_message))
+
+
+def test_extract_errors_returns_first_matching_pattern() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import SupersetErrorType
+
+ msg = "line 1:8: Table 'x' does not exist and Column 'bar' cannot be resolved"
+ result = PrestoEngineSpec.extract_errors(Exception(msg))
+
+ assert len(result) == 1
+ assert result[0].error_type == SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR
+
+
+def test_extract_errors_falls_back_to_generic_error() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import ErrorLevel, SupersetErrorType
+
+ result = PrestoEngineSpec.extract_errors(Exception("Generic Error"))
+
+ assert len(result) == 1
+ assert result[0].error_type == SupersetErrorType.GENERIC_DB_ENGINE_ERROR
+ assert result[0].message == "Generic Error"
+ assert result[0].level == ErrorLevel.ERROR
+ assert result[0].extra is not None
+ assert result[0].extra["engine_name"] == "Presto"
+ assert result[0].extra["issue_codes"] == [
+ {
+ "code": 1002,
+ "message": "Issue 1002 - The database returned an unexpected error.",
+ }
+ ]
+
+
+def test_extract_error_message_from_orig_database_error() -> None:
+ from collections import namedtuple
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ DatabaseError = namedtuple("DatabaseError", ["error_dict"]) # noqa: N806
+ db_err = DatabaseError(
+ {"errorName": "name", "errorLocation": "location", "message": "msg"}
+ )
+ exception = Exception()
+ exception.orig = db_err # type: ignore[attr-defined]
+
+ assert PrestoEngineSpec._extract_error_message(exception) == "name at location: msg"
+
+
+def test_extract_error_message_from_database_error_args() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ exception = DatabaseError({"message": "Err message"})
+
+ assert PrestoEngineSpec._extract_error_message(exception) == "Err message"
+
+
+def test_extract_error_message_from_database_error_without_message() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ exception = DatabaseError({"errorName": "SYNTAX_ERROR"})
+
+ assert str(PrestoEngineSpec._extract_error_message(exception)) == (
+ "Unknown Presto Error"
+ )
+
+
+def test_extract_error_message_from_general_exception() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert (
+ PrestoEngineSpec._extract_error_message(Exception("Err message"))
+ == "Err message"
+ )
+
+
+def test_expand_data_returns_input_untouched_when_flag_disabled() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "row_column",
+ "name": "row_column",
+ "type": "ROW(NESTED_OBJ VARCHAR)",
+ "is_dttm": False,
+ }
+ ]
+ data = [{"row_column": ["a"]}]
+
+ result_columns, result_data, expanded = PrestoEngineSpec.expand_data(columns, data)
+
+ assert result_columns is columns
+ assert result_data is data
+ assert expanded == []
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_flattens_deeply_nested_row_columns() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "r",
+ "name": "r",
+ "type": "ROW(L1 ROW(L2 ROW(L3 ROW(L4 VARCHAR))))",
+ "is_dttm": False,
+ }
+ ]
+ data = [{"r": [[[["deep"]]]]}]
+
+ result_columns, result_data, expanded = PrestoEngineSpec.expand_data(columns, data)
+
+ assert [column["column_name"] for column in result_columns] == [
+ "r",
+ "r.l1",
+ "r.l1.l2",
+ "r.l1.l2.l3",
+ "r.l1.l2.l3.l4",
+ ]
+ assert [column["column_name"] for column in expanded] == [
+ "r.l1",
+ "r.l1.l2",
+ "r.l1.l2.l3",
+ "r.l1.l2.l3.l4",
+ ]
+ assert result_data[0]["r.l1.l2.l3.l4"] == "deep"
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_raises_on_malformed_json_in_array_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "array_column",
+ "name": "array_column",
+ "type": "ARRAY(BIGINT)",
+ "is_dttm": False,
+ }
+ ]
+
+ with pytest.raises(ValueError, match="Expecting value"):
+ PrestoEngineSpec.expand_data(columns, [{"array_column": "not json"}])
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_raises_on_malformed_json_in_row_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "row_column",
+ "name": "row_column",
+ "type": "ROW(NESTED_OBJ VARCHAR)",
+ "is_dttm": False,
+ }
+ ]
+
+ with pytest.raises(ValueError, match="Expecting value"):
+ PrestoEngineSpec.expand_data(columns, [{"row_column": "not json"}])
+
+
+def test_get_function_names_lists_presto_functions() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame(
+ {"Function": ["abs", "avg", "cardinality"]}
+ )
+
+ assert PrestoEngineSpec.get_function_names(database) == [
+ "abs",
+ "avg",
+ "cardinality",
+ ]
+ database.get_df.assert_called_once_with("SHOW FUNCTIONS")
+
+
+def test_get_function_names_returns_empty_list_for_no_functions() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame({"Function": []})
+
+ assert PrestoEngineSpec.get_function_names(database) == []
+
+
+def test_get_function_names_raises_on_dataframe_without_function_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame()
+
+ with pytest.raises(KeyError, match="Function"):
+ PrestoEngineSpec.get_function_names(database)
+
+
+def test_get_function_names_propagates_connection_error() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.side_effect = Exception("Connection refused")
+
+ with pytest.raises(Exception, match="Connection refused"):
+ PrestoEngineSpec.get_function_names(database)
+
+
+@pytest.mark.parametrize(
+ "extra,expected",
+ [
+ pytest.param({}, False, id="no_version_key"),
+ pytest.param({"version": None}, False, id="version_none"),
+ pytest.param({"version": "0.318"}, False, id="just_below_gate"),
+ pytest.param({"version": "0.319"}, True, id="exactly_at_gate"),
+ pytest.param({"version": "0.400"}, True, id="above_gate"),
+ ],
+)
+def test_get_allow_cost_estimate_version_gate(
+ extra: dict[str, Any],
+ expected: bool,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_allow_cost_estimate(extra) is expected
+
+
+def test_get_allow_cost_estimate_rejects_unparseable_version() -> None:
+ from packaging.version import InvalidVersion
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(InvalidVersion):
+ PrestoEngineSpec.get_allow_cost_estimate({"version": "not-a-version"})
+
+
+def test_estimate_statement_cost() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = mock.MagicMock()
+ cursor.fetchone.return_value = ['{"a": "b"}']
+
+ result = PrestoEngineSpec.estimate_statement_cost(
+ mock.MagicMock(), "SELECT * FROM birth_names", cursor
+ )
+
+ assert result == {"a": "b"}
+ cursor.execute.assert_called_once_with(
+ "EXPLAIN (TYPE IO, FORMAT JSON) SELECT * FROM birth_names"
+ )
+
+
+def test_estimate_statement_cost_propagates_execute_failure() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = mock.MagicMock()
+ cursor.execute.side_effect = Exception("line 1:1: mismatched input 'DROP'")
+
+ with pytest.raises(Exception, match="mismatched input"):
+ PrestoEngineSpec.estimate_statement_cost(
+ mock.MagicMock(), "DROP TABLE birth_names", cursor
+ )
+
+
+def test_query_cost_formatter() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ raw_cost = [
+ {
+ "estimate": {
+ "outputRowCount": 9.04969899e8,
+ "outputSizeInBytes": 3.54143678301e11,
+ "cpuCost": 3.54143678301e11,
+ "maxMemory": 0.0,
+ "networkCost": 3.54143678301e11,
+ },
+ }
+ ]
+
+ assert PrestoEngineSpec.query_cost_formatter(raw_cost) == [
+ {
+ "Output count": "904 M rows",
+ "Output size": "354 GB",
+ "CPU cost": "354 G",
+ "Max memory": "0 B",
+ "Network cost": "354 G",
+ }
+ ]
+
+
+def test_query_cost_formatter_omits_missing_estimate_keys() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ raw_cost = [{"estimate": {"outputRowCount": 1234.0}}, {}]
+
+ assert PrestoEngineSpec.query_cost_formatter(raw_cost) == [
+ {"Output count": "1 K rows"},
+ {},
+ ]
+
+
+def test_query_cost_formatter_raises_on_null_estimate_value() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(TypeError):
+ PrestoEngineSpec.query_cost_formatter(
+ [{"estimate": {"outputRowCount": None, "outputSizeInBytes": 1.0}}]
+ )
+
+
+def test_estimate_query_cost_raises_when_version_too_old(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mocker.MagicMock()
+ database.get_extra.return_value = {"version": "0.318"}
+
+ with pytest.raises(Exception, match="Database does not support cost estimation"):
+ PrestoEngineSpec.estimate_query_cost(
+ database, "hive", "default", "SELECT 1", None
+ )
+
+ database.get_raw_connection.assert_not_called()
+
+
+def test_estimate_query_cost_estimates_each_statement(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mocker.MagicMock()
+ database.get_extra.return_value = {"version": "0.400"}
+ database.mutate_sql_based_on_config.side_effect = lambda sql, **_kwargs: sql
+ cursor = mock.MagicMock()
+ cursor.fetchone.side_effect = [
+ ['{"estimate": {"outputRowCount": 1.0}}'],
+ ['{"estimate": {"outputRowCount": 2.0}}'],
+ ]
+ database.get_raw_connection.return_value.__enter__.return_value.cursor.return_value = ( # noqa: E501
+ cursor
+ )
+
+ result = PrestoEngineSpec.estimate_query_cost(
+ database, "hive", "default", "SELECT 1; SELECT 2", None
+ )
+
+ assert result == [
+ {"estimate": {"outputRowCount": 1.0}},
+ {"estimate": {"outputRowCount": 2.0}},
+ ]
+ assert cursor.execute.call_args_list == [
+ mock.call("EXPLAIN (TYPE IO, FORMAT JSON) SELECT\n 1"),
+ mock.call("EXPLAIN (TYPE IO, FORMAT JSON) SELECT\n 2"),
+ ]
+
+
+TRACKING_URL = (
+ "https://presto.example.com:8080/ui/query.html?20220101_120000_00001_abcde"
+)
+
+
+def _presto_cursor() -> mock.MagicMock:
+ cursor = mock.MagicMock()
+ cursor._protocol = "https"
+ cursor._host = "presto.example.com"
+ cursor._port = 8080
+ cursor.last_query_id = "20220101_120000_00001_abcde"
+ return cursor
+
+
+def _handle_cursor_query(
+ mocker: MockerFixture,
+) -> tuple[mock.MagicMock, mock.MagicMock]:
+ from superset.common.db_query_status import QueryStatus
+
+ mock_db = mocker.patch("superset.db_engine_specs.presto.db")
+ query = mock.MagicMock()
+ query.id = 42
+ query.progress = 0
+ query.status = QueryStatus.RUNNING
+ query.database.connect_args = {"poll_interval": 0}
+ mock_db.session.query.return_value.filter_by.return_value.one.return_value = query
+ return mock_db, query
+
+
+def test_get_tracking_url_builds_presto_ui_link() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_tracking_url(_presto_cursor()) == TRACKING_URL
+
+
+def test_get_tracking_url_returns_none_for_falsy_query_id() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = _presto_cursor()
+ cursor.last_query_id = None
+
+ assert PrestoEngineSpec.get_tracking_url(cursor) is None
+
+
+def test_get_tracking_url_returns_none_when_attribute_absent() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_tracking_url(mock.Mock(spec=[])) is None
+
+
+def test_handle_cursor_records_tracking_url_and_progress(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ mock_db, query = _handle_cursor_query(mocker)
+ cursor = _presto_cursor()
+ cursor.poll.side_effect = [
+ {"stats": {"state": "RUNNING", "completedSplits": 5, "totalSplits": 10}},
+ None,
+ ]
+
+ PrestoEngineSpec.handle_cursor(cursor, query)
+
+ assert query.tracking_url == TRACKING_URL
+ assert query.progress == 50.0
+ assert cursor.poll.call_count == 2
+ cursor.cancel.assert_not_called()
+ assert mock_db.session.commit.called
+
+
+def test_handle_cursor_stops_polling_when_query_finished(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ _mock_db, query = _handle_cursor_query(mocker)
+ cursor = _presto_cursor()
+ cursor.poll.side_effect = [{"stats": {"state": "FINISHED"}}]
+
+ PrestoEngineSpec.handle_cursor(cursor, query)
+
+ assert query.progress == 0
+ assert cursor.poll.call_count == 1
+ cursor.cancel.assert_not_called()
+
+
+@pytest.mark.parametrize("status", ["STOPPED", "TIMED_OUT"])
+def test_handle_cursor_cancels_when_user_stops_query(
+ mocker: MockerFixture,
+ status: str,
+) -> None:
+ from superset.common.db_query_status import QueryStatus
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ _mock_db, query = _handle_cursor_query(mocker)
+ query.status = getattr(QueryStatus, status)
Review Comment:
This changes the same object returned by the mocked session before `handle_cursor` runs, so it would still pass if the loop stopped reloading the query after another request marks it stopped. That is the boundary that releases the live Presto query. Could this use distinct initial `RUNNING` and reloaded `STOPPED`/`TIMED_OUT` objects and assert the reload before cancellation?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "drivaspreset (via GitHub)" <gi...@apache.org>.
drivaspreset commented on code in PR #43607:
URL: https://github.com/apache/superset/pull/43607#discussion_r3883110488
##########
tests/unit_tests/db_engine_specs/test_presto.py:
##########
@@ -412,6 +656,635 @@ def test_extract_errors_maps_401_to_access_denied() -> None:
assert result[0].error_type == SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR
+@pytest.mark.parametrize(
+ "raw_message,context,expected_error_type,expected_message",
+ [
+ pytest.param(
+ "line 1:8: Column 'bar' cannot be resolved",
+ {},
+ "COLUMN_DOES_NOT_EXIST_ERROR",
+ 'We can\'t seem to resolve the column "bar" at line 1:8.',
+ id="column_does_not_exist",
+ ),
+ pytest.param(
+ "Table 'default.foo' does not exist",
+ {},
+ "TABLE_DOES_NOT_EXIST_ERROR",
+ "The table \"'default.foo'\" does not exist. "
+ "A valid table must be used to run this query.",
+ id="table_does_not_exist",
+ ),
+ pytest.param(
+ "line 1:15: Schema 'bar' does not exist",
+ {},
+ "SCHEMA_DOES_NOT_EXIST_ERROR",
+ 'The schema "bar" does not exist. '
+ "A valid schema must be used to run this query.",
+ id="schema_does_not_exist",
+ ),
+ pytest.param(
+ "Access Denied: Invalid credentials",
+ {"username": "bob"},
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ 'Either the username "bob" or the password is incorrect.',
+ id="access_denied_invalid_credentials",
+ ),
+ pytest.param(
+ "presto error: Unexpected status code 401 b'Unauthorized'",
+ {},
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ "Unexpected HTTP 401 response. Check your credentials.",
+ id="access_denied_http_401",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 8] nodename nor "
+ "servname provided, or not known",
+ {"hostname": "badhost"},
+ "CONNECTION_INVALID_HOSTNAME_ERROR",
+ 'The hostname "badhost" cannot be resolved.',
+ id="invalid_hostname",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 60] Operation timed out",
+ {"hostname": "myhost", "port": 8080},
+ "CONNECTION_HOST_DOWN_ERROR",
+ 'The host "myhost" might be down, and can\'t be reached on port 8080.',
+ id="host_down_operation_timed_out",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 61] Connection refused",
+ {"hostname": "myhost", "port": 8080},
+ "CONNECTION_PORT_CLOSED_ERROR",
+ 'Port 8080 on hostname "myhost" refused the connection.',
+ id="port_closed",
+ ),
+ pytest.param(
+ "line 1:8: Catalog 'foo' does not exist",
+ {},
+ "CONNECTION_UNKNOWN_DATABASE_ERROR",
+ 'Unable to connect to catalog named "foo".',
+ id="unknown_catalog",
+ ),
+ ],
+)
+def test_extract_errors_matches_all_custom_error_patterns(
+ raw_message: str,
+ context: dict[str, Any],
+ expected_error_type: str,
+ expected_message: str,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import ErrorLevel, SupersetErrorType
+
+ result = PrestoEngineSpec.extract_errors(Exception(raw_message), context=context)
+
+ assert len(result) == 1
+ assert result[0].error_type == getattr(SupersetErrorType, expected_error_type)
+ assert result[0].message == expected_message
+ assert result[0].level == ErrorLevel.ERROR
+ assert result[0].extra is not None
+ assert result[0].extra["engine_name"] == "Presto"
+
+
+@pytest.mark.parametrize(
+ "raw_message,missing_placeholder",
+ [
+ pytest.param(
+ "Access Denied: Invalid credentials", "username", id="access_denied"
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 8] nodename nor "
+ "servname provided, or not known",
+ "hostname",
+ id="invalid_hostname",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 60] Operation timed out",
+ "hostname",
+ id="host_down",
+ ),
+ pytest.param(
+ "Failed to establish a new connection: [Errno 61] Connection refused",
+ "port",
+ id="port_closed",
+ ),
+ ],
+)
+def test_extract_errors_raises_key_error_without_context(
+ raw_message: str,
+ missing_placeholder: str,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(KeyError, match=missing_placeholder):
+ PrestoEngineSpec.extract_errors(Exception(raw_message))
+
+
+def test_extract_errors_returns_first_matching_pattern() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import SupersetErrorType
+
+ msg = "line 1:8: Table 'x' does not exist and Column 'bar' cannot be resolved"
+ result = PrestoEngineSpec.extract_errors(Exception(msg))
+
+ assert len(result) == 1
+ assert result[0].error_type == SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR
+
+
+def test_extract_errors_falls_back_to_generic_error() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+ from superset.errors import ErrorLevel, SupersetErrorType
+
+ result = PrestoEngineSpec.extract_errors(Exception("Generic Error"))
+
+ assert len(result) == 1
+ assert result[0].error_type == SupersetErrorType.GENERIC_DB_ENGINE_ERROR
+ assert result[0].message == "Generic Error"
+ assert result[0].level == ErrorLevel.ERROR
+ assert result[0].extra is not None
+ assert result[0].extra["engine_name"] == "Presto"
+ assert result[0].extra["issue_codes"] == [
+ {
+ "code": 1002,
+ "message": "Issue 1002 - The database returned an unexpected error.",
+ }
+ ]
+
+
+def test_extract_error_message_from_orig_database_error() -> None:
+ from collections import namedtuple
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ DatabaseError = namedtuple("DatabaseError", ["error_dict"]) # noqa: N806
+ db_err = DatabaseError(
+ {"errorName": "name", "errorLocation": "location", "message": "msg"}
+ )
+ exception = Exception()
+ exception.orig = db_err # type: ignore[attr-defined]
+
+ assert PrestoEngineSpec._extract_error_message(exception) == "name at location: msg"
+
+
+def test_extract_error_message_from_database_error_args() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ exception = DatabaseError({"message": "Err message"})
+
+ assert PrestoEngineSpec._extract_error_message(exception) == "Err message"
+
+
+def test_extract_error_message_from_database_error_without_message() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ exception = DatabaseError({"errorName": "SYNTAX_ERROR"})
+
+ assert str(PrestoEngineSpec._extract_error_message(exception)) == (
+ "Unknown Presto Error"
+ )
+
+
+def test_extract_error_message_from_general_exception() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert (
+ PrestoEngineSpec._extract_error_message(Exception("Err message"))
+ == "Err message"
+ )
+
+
+def test_expand_data_returns_input_untouched_when_flag_disabled() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "row_column",
+ "name": "row_column",
+ "type": "ROW(NESTED_OBJ VARCHAR)",
+ "is_dttm": False,
+ }
+ ]
+ data = [{"row_column": ["a"]}]
+
+ result_columns, result_data, expanded = PrestoEngineSpec.expand_data(columns, data)
+
+ assert result_columns is columns
+ assert result_data is data
+ assert expanded == []
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_flattens_deeply_nested_row_columns() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "r",
+ "name": "r",
+ "type": "ROW(L1 ROW(L2 ROW(L3 ROW(L4 VARCHAR))))",
+ "is_dttm": False,
+ }
+ ]
+ data = [{"r": [[[["deep"]]]]}]
+
+ result_columns, result_data, expanded = PrestoEngineSpec.expand_data(columns, data)
+
+ assert [column["column_name"] for column in result_columns] == [
+ "r",
+ "r.l1",
+ "r.l1.l2",
+ "r.l1.l2.l3",
+ "r.l1.l2.l3.l4",
+ ]
+ assert [column["column_name"] for column in expanded] == [
+ "r.l1",
+ "r.l1.l2",
+ "r.l1.l2.l3",
+ "r.l1.l2.l3.l4",
+ ]
+ assert result_data[0]["r.l1.l2.l3.l4"] == "deep"
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_raises_on_malformed_json_in_array_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "array_column",
+ "name": "array_column",
+ "type": "ARRAY(BIGINT)",
+ "is_dttm": False,
+ }
+ ]
+
+ with pytest.raises(ValueError, match="Expecting value"):
+ PrestoEngineSpec.expand_data(columns, [{"array_column": "not json"}])
+
+
+@mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ {"PRESTO_EXPAND_DATA": True},
+ clear=True,
+)
+def test_expand_data_raises_on_malformed_json_in_row_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ columns: list[ResultSetColumnType] = [
+ {
+ "column_name": "row_column",
+ "name": "row_column",
+ "type": "ROW(NESTED_OBJ VARCHAR)",
+ "is_dttm": False,
+ }
+ ]
+
+ with pytest.raises(ValueError, match="Expecting value"):
+ PrestoEngineSpec.expand_data(columns, [{"row_column": "not json"}])
+
+
+def test_get_function_names_lists_presto_functions() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame(
+ {"Function": ["abs", "avg", "cardinality"]}
+ )
+
+ assert PrestoEngineSpec.get_function_names(database) == [
+ "abs",
+ "avg",
+ "cardinality",
+ ]
+ database.get_df.assert_called_once_with("SHOW FUNCTIONS")
+
+
+def test_get_function_names_returns_empty_list_for_no_functions() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame({"Function": []})
+
+ assert PrestoEngineSpec.get_function_names(database) == []
+
+
+def test_get_function_names_raises_on_dataframe_without_function_column() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.return_value = pd.DataFrame()
+
+ with pytest.raises(KeyError, match="Function"):
+ PrestoEngineSpec.get_function_names(database)
+
+
+def test_get_function_names_propagates_connection_error() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_df.side_effect = Exception("Connection refused")
+
+ with pytest.raises(Exception, match="Connection refused"):
+ PrestoEngineSpec.get_function_names(database)
+
+
+@pytest.mark.parametrize(
+ "extra,expected",
+ [
+ pytest.param({}, False, id="no_version_key"),
+ pytest.param({"version": None}, False, id="version_none"),
+ pytest.param({"version": "0.318"}, False, id="just_below_gate"),
+ pytest.param({"version": "0.319"}, True, id="exactly_at_gate"),
+ pytest.param({"version": "0.400"}, True, id="above_gate"),
+ ],
+)
+def test_get_allow_cost_estimate_version_gate(
+ extra: dict[str, Any],
+ expected: bool,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_allow_cost_estimate(extra) is expected
+
+
+def test_get_allow_cost_estimate_rejects_unparseable_version() -> None:
+ from packaging.version import InvalidVersion
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(InvalidVersion):
+ PrestoEngineSpec.get_allow_cost_estimate({"version": "not-a-version"})
+
+
+def test_estimate_statement_cost() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = mock.MagicMock()
+ cursor.fetchone.return_value = ['{"a": "b"}']
+
+ result = PrestoEngineSpec.estimate_statement_cost(
+ mock.MagicMock(), "SELECT * FROM birth_names", cursor
+ )
+
+ assert result == {"a": "b"}
+ cursor.execute.assert_called_once_with(
+ "EXPLAIN (TYPE IO, FORMAT JSON) SELECT * FROM birth_names"
+ )
+
+
+def test_estimate_statement_cost_propagates_execute_failure() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = mock.MagicMock()
+ cursor.execute.side_effect = Exception("line 1:1: mismatched input 'DROP'")
+
+ with pytest.raises(Exception, match="mismatched input"):
+ PrestoEngineSpec.estimate_statement_cost(
+ mock.MagicMock(), "DROP TABLE birth_names", cursor
+ )
+
+
+def test_query_cost_formatter() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ raw_cost = [
+ {
+ "estimate": {
+ "outputRowCount": 9.04969899e8,
+ "outputSizeInBytes": 3.54143678301e11,
+ "cpuCost": 3.54143678301e11,
+ "maxMemory": 0.0,
+ "networkCost": 3.54143678301e11,
+ },
+ }
+ ]
+
+ assert PrestoEngineSpec.query_cost_formatter(raw_cost) == [
+ {
+ "Output count": "904 M rows",
+ "Output size": "354 GB",
+ "CPU cost": "354 G",
+ "Max memory": "0 B",
+ "Network cost": "354 G",
+ }
+ ]
+
+
+def test_query_cost_formatter_omits_missing_estimate_keys() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ raw_cost = [{"estimate": {"outputRowCount": 1234.0}}, {}]
+
+ assert PrestoEngineSpec.query_cost_formatter(raw_cost) == [
+ {"Output count": "1 K rows"},
+ {},
+ ]
+
+
+def test_query_cost_formatter_raises_on_null_estimate_value() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ with pytest.raises(TypeError):
+ PrestoEngineSpec.query_cost_formatter(
+ [{"estimate": {"outputRowCount": None, "outputSizeInBytes": 1.0}}]
+ )
+
+
+def test_estimate_query_cost_raises_when_version_too_old(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mocker.MagicMock()
+ database.get_extra.return_value = {"version": "0.318"}
+
+ with pytest.raises(Exception, match="Database does not support cost estimation"):
+ PrestoEngineSpec.estimate_query_cost(
+ database, "hive", "default", "SELECT 1", None
+ )
+
+ database.get_raw_connection.assert_not_called()
+
+
+def test_estimate_query_cost_estimates_each_statement(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mocker.MagicMock()
+ database.get_extra.return_value = {"version": "0.400"}
+ database.mutate_sql_based_on_config.side_effect = lambda sql, **_kwargs: sql
+ cursor = mock.MagicMock()
+ cursor.fetchone.side_effect = [
+ ['{"estimate": {"outputRowCount": 1.0}}'],
+ ['{"estimate": {"outputRowCount": 2.0}}'],
+ ]
+ database.get_raw_connection.return_value.__enter__.return_value.cursor.return_value = ( # noqa: E501
+ cursor
+ )
+
+ result = PrestoEngineSpec.estimate_query_cost(
+ database, "hive", "default", "SELECT 1; SELECT 2", None
+ )
+
+ assert result == [
+ {"estimate": {"outputRowCount": 1.0}},
+ {"estimate": {"outputRowCount": 2.0}},
+ ]
+ assert cursor.execute.call_args_list == [
+ mock.call("EXPLAIN (TYPE IO, FORMAT JSON) SELECT\n 1"),
+ mock.call("EXPLAIN (TYPE IO, FORMAT JSON) SELECT\n 2"),
+ ]
+
+
+TRACKING_URL = (
+ "https://presto.example.com:8080/ui/query.html?20220101_120000_00001_abcde"
+)
+
+
+def _presto_cursor() -> mock.MagicMock:
+ cursor = mock.MagicMock()
+ cursor._protocol = "https"
+ cursor._host = "presto.example.com"
+ cursor._port = 8080
+ cursor.last_query_id = "20220101_120000_00001_abcde"
+ return cursor
+
+
+def _handle_cursor_query(
+ mocker: MockerFixture,
+) -> tuple[mock.MagicMock, mock.MagicMock]:
+ from superset.common.db_query_status import QueryStatus
+
+ mock_db = mocker.patch("superset.db_engine_specs.presto.db")
+ query = mock.MagicMock()
+ query.id = 42
+ query.progress = 0
+ query.status = QueryStatus.RUNNING
+ query.database.connect_args = {"poll_interval": 0}
+ mock_db.session.query.return_value.filter_by.return_value.one.return_value = query
+ return mock_db, query
+
+
+def test_get_tracking_url_builds_presto_ui_link() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_tracking_url(_presto_cursor()) == TRACKING_URL
+
+
+def test_get_tracking_url_returns_none_for_falsy_query_id() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ cursor = _presto_cursor()
+ cursor.last_query_id = None
+
+ assert PrestoEngineSpec.get_tracking_url(cursor) is None
+
+
+def test_get_tracking_url_returns_none_when_attribute_absent() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ assert PrestoEngineSpec.get_tracking_url(mock.Mock(spec=[])) is None
+
+
+def test_handle_cursor_records_tracking_url_and_progress(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ mock_db, query = _handle_cursor_query(mocker)
+ cursor = _presto_cursor()
+ cursor.poll.side_effect = [
+ {"stats": {"state": "RUNNING", "completedSplits": 5, "totalSplits": 10}},
+ None,
+ ]
+
+ PrestoEngineSpec.handle_cursor(cursor, query)
+
+ assert query.tracking_url == TRACKING_URL
+ assert query.progress == 50.0
+ assert cursor.poll.call_count == 2
+ cursor.cancel.assert_not_called()
+ assert mock_db.session.commit.called
+
+
+def test_handle_cursor_stops_polling_when_query_finished(
+ mocker: MockerFixture,
+) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ _mock_db, query = _handle_cursor_query(mocker)
+ cursor = _presto_cursor()
+ cursor.poll.side_effect = [{"stats": {"state": "FINISHED"}}]
+
+ PrestoEngineSpec.handle_cursor(cursor, query)
+
+ assert query.progress == 0
+ assert cursor.poll.call_count == 1
+ cursor.cancel.assert_not_called()
+
+
+@pytest.mark.parametrize("status", ["STOPPED", "TIMED_OUT"])
+def test_handle_cursor_cancels_when_user_stops_query(
+ mocker: MockerFixture,
+ status: str,
+) -> None:
+ from superset.common.db_query_status import QueryStatus
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ _mock_db, query = _handle_cursor_query(mocker)
+ query.status = getattr(QueryStatus, status)
Review Comment:
Yes you're right about the boundary. I confirmed it by mutation: with the db.session.query(...).filter_by(id=query_id).one() reload removed from handle_cursor, the old test still passed
Updated the test: the query passed in stays RUNNING, the mocked session returns a distinct STOPPED/TIMED_OUT object, and the test asserts the reload happened with the right id and that it preceded cursor.cancel(). With the reload removed it now fails on both statuses
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5456320562
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | c4e140f5deff7066b82a2b9a0b16cd748a2a9c57 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a91d44ab128800008d26c5f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43607--superset-docs-preview.netlify.app](https://deploy-preview-43607--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#FYtLCoAwDAXvkrUBFZXqDTxG28QPaBvauhDx7sZZPAYe84CkeEqBCaACl2zwm3rhXFCSbkRyyGHdA2MW9niFveD_Zw2I5Yj3TJoMdmyo66xrWmNqxVA7-H6B9wM) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43607--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5444169123
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43607?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 66.34%. Comparing base ([`76151be`](https://app.codecov.io/gh/apache/superset/commit/76151beade203694ff1d5eb7579e8e584a6220cc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4f2392f`](https://app.codecov.io/gh/apache/superset/commit/4f2392fe7cd18bed0170c8058beda8537e681b32?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (76151be) and HEAD (4f2392f). Click for more details.
>
> <details><summary>HEAD has 50 uploads less than BASE</summary>
>
>| Flag | BASE (76151be) | HEAD (4f2392f) |
>|------|------|------|
>|python|37|3|
>|sqlite|6|1|
>|postgres|7|1|
>|mysql|6|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43607 +/- ##
===========================================
- Coverage 79.13% 66.34% -12.80%
===========================================
Files 2879 2879
Lines 165764 165764
Branches 38320 38320
===========================================
- Hits 131178 109968 -21210
- Misses 32103 53577 +21474
+ Partials 2483 2219 -264
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.68% <ø> (ø)` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.71% <ø> (-0.01%)` | :arrow_down: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.92% <ø> (-25.80%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.40% <ø> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43607/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43607?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "drivaspreset (via GitHub)" <gi...@apache.org>.
drivaspreset commented on code in PR #43607:
URL: https://github.com/apache/superset/pull/43607#discussion_r3883099485
##########
tests/unit_tests/db_engine_specs/test_presto.py:
##########
@@ -113,6 +160,203 @@ def test_get_schema_from_engine_params() -> None:
)
+@pytest.mark.parametrize(
+ "schema",
+ [
+ pytest.param("with/slash", id="slash"),
+ pytest.param("with space", id="space"),
+ pytest.param("with%percent", id="percent"),
+ pytest.param("地区", id="unicode"),
+ pytest.param("plain", id="plain"),
+ ],
+)
+def test_schema_survives_engine_params_round_trip(schema: str) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ uri, _connect_args = PrestoEngineSpec.adjust_engine_params(
+ make_url("presto://localhost:8080/hive"),
+ {},
+ schema=schema,
+ )
+
+ assert PrestoEngineSpec.get_schema_from_engine_params(uri, {}) == schema
+
+
+def test_get_catalog_names_lists_catalogs() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ conn = inspector.engine.connect.return_value.__enter__.return_value
+ conn.execute.return_value = [("jmx",), ("tpch",), ("memory",)]
+
+ result = PrestoEngineSpec.get_catalog_names(mock.MagicMock(), inspector)
+
+ assert result == {"jmx", "tpch", "memory"}
+ assert str(conn.execute.call_args[0][0]) == "SHOW CATALOGS"
+
+
+def test_get_view_names_queries_information_schema_with_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["a", "b,", "c"], ["d", "e"]]
+
+ result = PrestoEngineSpec.get_view_names(database, mock.Mock(), "my_schema")
+
+ assert result == {"a", "d"}
+ cursor.execute.assert_called_once_with(
+ dedent(
+ """
+ SELECT table_name FROM information_schema.tables
+ WHERE table_schema = %(schema)s
+ AND table_type = 'VIEW'
+ """
+ ).strip(),
+ {"schema": "my_schema"},
+ )
+
+
+def test_get_view_names_queries_information_schema_without_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["a", "b,", "c"], ["d", "e"]]
+
+ result = PrestoEngineSpec.get_view_names(database, mock.Mock(), None)
+
+ assert result == {"a", "d"}
+ cursor.execute.assert_called_once_with(
+ dedent(
+ """
+ SELECT table_name FROM information_schema.tables
+ WHERE table_type = 'VIEW'
+ """
+ ).strip(),
+ {},
+ )
+
+
+def test_get_view_names_returns_empty_set_when_no_views() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = []
+
+ assert PrestoEngineSpec.get_view_names(database, mock.Mock(), "empty") == set()
+
+
+def test_get_view_names_propagates_driver_error() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().execute.side_effect = (
+ DatabaseError("Access Denied: Cannot select from table information_schema")
+ )
+
+ with pytest.raises(DatabaseError, match="Access Denied"):
+ PrestoEngineSpec.get_view_names(database, mock.Mock(), "my_schema")
+
+
+def test_get_table_names_subtracts_views() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ inspector.get_table_names.return_value = ["t1", "t2", "v1", "v2"]
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = [
+ ["v1"],
+ ["v2"],
+ ]
+
+ result = PrestoEngineSpec.get_table_names(database, inspector, "my_schema")
+
+ assert result == {"t1", "t2"}
+
+
+def test_get_table_names_returns_empty_set_for_empty_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ inspector.get_table_names.return_value = []
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = []
+
+ assert PrestoEngineSpec.get_table_names(database, inspector, "empty") == set()
+
+
+def test_get_create_view_returns_view_definition() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["CREATE VIEW v AS SELECT 1", "b"], ["d"]]
+
+ result = PrestoEngineSpec.get_create_view(database, schema="s", table="v")
+
+ assert result == "CREATE VIEW v AS SELECT 1"
+
+
+@pytest.mark.parametrize(
+ "schema,table,expected_sql",
+ [
+ pytest.param("s", "v", "SHOW CREATE VIEW s.v", id="simple"),
+ pytest.param(
+ "analytics",
+ "daily_users",
+ "SHOW CREATE VIEW analytics.daily_users",
+ id="schema_qualified",
+ ),
+ pytest.param(
+ "s",
+ 'v" OR 1=1',
Review Comment:
Agreed, that case encoded "identifiers aren't validated" as expected behaviour, which would turn a future quoting or validation fix into a red test that looks like a regression.
Swapped it for a valid qualified identifier as you suggested: Raw_2024.Daily_Active_Users_v2, which still covers something the other two cases don't, that schema and table are passed through without normalisation (no lower-casing).
On the bot's suggestion of parameterised queries: identifiers can't be bound as parameters in SQL, only values can. The equivalent fix would be identifier quoting via the dialect's preparer, which is a production change and out of scope for this test-only PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #43607:
URL: https://github.com/apache/superset/pull/43607
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5445461854
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5450679205
<!-- Bito Reply -->
The concern regarding identifiers containing `"` in `SHOW CREATE VIEW` is valid. Relying on raw interpolation for such identifiers can lead to regressions if future validation or quoting logic is introduced, as it conflates malformed input with expected behavior.
To address this, it is recommended to:
1. Use parameterized queries for all identifier handling to ensure proper escaping and prevent SQL injection or syntax errors.
2. Separate the testing of valid qualified identifiers from the testing of malformed identifiers, ensuring the latter is explicitly validated against a defined contract rather than implicitly handled by the engine.
**tests/unit_tests/db_engine_specs/test_presto.py**
```
cursor.execute.assert_called_once_with(
dedent(
"""
SELECT table_name FROM information_schema.tables
WHERE table_schema = %(schema)s
AND table_type = 'VIEW'
"""
).strip(),
{"schema": "my_schema"},
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43607:
URL: https://github.com/apache/superset/pull/43607#discussion_r3879388638
##########
tests/unit_tests/db_engine_specs/test_presto.py:
##########
@@ -113,6 +160,203 @@ def test_get_schema_from_engine_params() -> None:
)
+@pytest.mark.parametrize(
+ "schema",
+ [
+ pytest.param("with/slash", id="slash"),
+ pytest.param("with space", id="space"),
+ pytest.param("with%percent", id="percent"),
+ pytest.param("地区", id="unicode"),
+ pytest.param("plain", id="plain"),
+ ],
+)
+def test_schema_survives_engine_params_round_trip(schema: str) -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ uri, _connect_args = PrestoEngineSpec.adjust_engine_params(
+ make_url("presto://localhost:8080/hive"),
+ {},
+ schema=schema,
+ )
+
+ assert PrestoEngineSpec.get_schema_from_engine_params(uri, {}) == schema
+
+
+def test_get_catalog_names_lists_catalogs() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ conn = inspector.engine.connect.return_value.__enter__.return_value
+ conn.execute.return_value = [("jmx",), ("tpch",), ("memory",)]
+
+ result = PrestoEngineSpec.get_catalog_names(mock.MagicMock(), inspector)
+
+ assert result == {"jmx", "tpch", "memory"}
+ assert str(conn.execute.call_args[0][0]) == "SHOW CATALOGS"
+
+
+def test_get_view_names_queries_information_schema_with_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["a", "b,", "c"], ["d", "e"]]
+
+ result = PrestoEngineSpec.get_view_names(database, mock.Mock(), "my_schema")
+
+ assert result == {"a", "d"}
+ cursor.execute.assert_called_once_with(
+ dedent(
+ """
+ SELECT table_name FROM information_schema.tables
+ WHERE table_schema = %(schema)s
+ AND table_type = 'VIEW'
+ """
+ ).strip(),
+ {"schema": "my_schema"},
+ )
+
+
+def test_get_view_names_queries_information_schema_without_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["a", "b,", "c"], ["d", "e"]]
+
+ result = PrestoEngineSpec.get_view_names(database, mock.Mock(), None)
+
+ assert result == {"a", "d"}
+ cursor.execute.assert_called_once_with(
+ dedent(
+ """
+ SELECT table_name FROM information_schema.tables
+ WHERE table_type = 'VIEW'
+ """
+ ).strip(),
+ {},
+ )
+
+
+def test_get_view_names_returns_empty_set_when_no_views() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = []
+
+ assert PrestoEngineSpec.get_view_names(database, mock.Mock(), "empty") == set()
+
+
+def test_get_view_names_propagates_driver_error() -> None:
+ from pyhive.exc import DatabaseError
+
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().execute.side_effect = (
+ DatabaseError("Access Denied: Cannot select from table information_schema")
+ )
+
+ with pytest.raises(DatabaseError, match="Access Denied"):
+ PrestoEngineSpec.get_view_names(database, mock.Mock(), "my_schema")
+
+
+def test_get_table_names_subtracts_views() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ inspector.get_table_names.return_value = ["t1", "t2", "v1", "v2"]
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = [
+ ["v1"],
+ ["v2"],
+ ]
+
+ result = PrestoEngineSpec.get_table_names(database, inspector, "my_schema")
+
+ assert result == {"t1", "t2"}
+
+
+def test_get_table_names_returns_empty_set_for_empty_schema() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ inspector = mock.MagicMock()
+ inspector.get_table_names.return_value = []
+ database = mock.MagicMock()
+ database.get_raw_connection().__enter__().cursor().fetchall.return_value = []
+
+ assert PrestoEngineSpec.get_table_names(database, inspector, "empty") == set()
+
+
+def test_get_create_view_returns_view_definition() -> None:
+ from superset.db_engine_specs.presto import PrestoEngineSpec
+
+ database = mock.MagicMock()
+ cursor = database.get_raw_connection().__enter__().cursor()
+ cursor.fetchall.return_value = [["CREATE VIEW v AS SELECT 1", "b"], ["d"]]
+
+ result = PrestoEngineSpec.get_create_view(database, schema="s", table="v")
+
+ assert result == "CREATE VIEW v AS SELECT 1"
+
+
+@pytest.mark.parametrize(
+ "schema,table,expected_sql",
+ [
+ pytest.param("s", "v", "SHOW CREATE VIEW s.v", id="simple"),
+ pytest.param(
+ "analytics",
+ "daily_users",
+ "SHOW CREATE VIEW analytics.daily_users",
+ id="schema_qualified",
+ ),
+ pytest.param(
+ "s",
+ 'v" OR 1=1',
Review Comment:
An identifier containing `"` makes this test require raw interpolation in `SHOW CREATE VIEW`. That turns a future validation or identifier-quoting fix into a regression while malformed identifiers remain an expected behavior. Could this case cover valid qualified identifiers instead, with malformed input tested only against an intentional validation contract?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5458035037
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: test(presto) close db_engine_spec unit test gaps [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43607:
URL: https://github.com/apache/superset/pull/43607#issuecomment-5443344603
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org