fix(maintenance): survive sqlparse token cap on huge virtual dataset SQL
Discovery of virtual datasets now works, but a runtime blocker remained: any virtual dataset whose SQL exceeds sqlparse's MAX_GROUPING_TOKENS (10000 tokens) raised SQLParseError 'Maximum number of tokens exceeded (10000)' from extract_tables_from_sql_span, which is called unguarded in the scan loop — one oversized virtual dataset aborted the whole maintenance preview/start. - extract_tables_from_sql_span now wraps sqlparse.parse + token walk in try/except and falls back to regex-only extraction (keeping all schema.table matches) instead of raising, so huge SQL no longer fails the scan. - Tier-1 virtual filter uses value "" (not None) so the sql is_not_null filter passes Superset's rison schema instead of always falling back to a full scan. Tests: huge-SQL fallback (extractor) and huge-virtual-dataset scan resilience (scanner). ADR-0020 updated with Decision 3.
This commit is contained in:
@@ -90,7 +90,7 @@ async def find_affected_dashboards(
|
||||
try:
|
||||
virtual_query = {
|
||||
"columns": ["id", "table_name", "schema", "sql"],
|
||||
"filters": [{"col": "sql", "opr": "is_not_null", "value": None}],
|
||||
"filters": [{"col": "sql", "opr": "is_not_null", "value": ""}],
|
||||
}
|
||||
_, virtual_datasets = await superset_client.get_datasets(query=virtual_query)
|
||||
except Exception as e:
|
||||
|
||||
@@ -146,28 +146,37 @@ def extract_tables_from_sql_span(sql_text: str) -> set[str]:
|
||||
if not raw_matches:
|
||||
return tables
|
||||
|
||||
# Use sqlparse to identify string literal positions
|
||||
parsed = sqlparse.parse(sql_text)
|
||||
# Use sqlparse to identify string literal positions. Some production virtual
|
||||
# datasets contain SQL so large that sqlparse refuses to group it
|
||||
# (MAX_GROUPING_TOKENS = 10000 tokens → SQLParseError). In that case fall back
|
||||
# to regex-only extraction (accepting potential string-literal false positives)
|
||||
# rather than failing the whole maintenance scan.
|
||||
string_literal_ranges: list[tuple[int, int]] = []
|
||||
try:
|
||||
parsed = sqlparse.parse(sql_text)
|
||||
|
||||
def walk_tokens(tokens: Iterable[Token], base_offset: int = 0) -> None:
|
||||
offset = base_offset
|
||||
for token in tokens:
|
||||
if isinstance(token, TokenList):
|
||||
walk_tokens(token.flatten(), offset)
|
||||
else:
|
||||
ttype = token.ttype
|
||||
val = token.value
|
||||
if is_string_literal(token):
|
||||
string_literal_ranges.append(
|
||||
(offset, offset + len(val))
|
||||
)
|
||||
offset += len(val)
|
||||
def walk_tokens(tokens: Iterable[Token], base_offset: int = 0) -> None:
|
||||
offset = base_offset
|
||||
for token in tokens:
|
||||
if isinstance(token, TokenList):
|
||||
walk_tokens(token.flatten(), offset)
|
||||
else:
|
||||
ttype = token.ttype
|
||||
val = token.value
|
||||
if is_string_literal(token):
|
||||
string_literal_ranges.append(
|
||||
(offset, offset + len(val))
|
||||
)
|
||||
offset += len(val)
|
||||
|
||||
for stmt in parsed:
|
||||
if stmt is None:
|
||||
continue
|
||||
walk_tokens(stmt.flatten(), base_offset=0)
|
||||
for stmt in parsed:
|
||||
if stmt is None:
|
||||
continue
|
||||
walk_tokens(stmt.flatten(), base_offset=0)
|
||||
except Exception:
|
||||
# sqlparse failed (e.g. token-limit) — treat no text as a string literal so
|
||||
# every regex match is kept. Best-effort matching over hard failure.
|
||||
string_literal_ranges = []
|
||||
|
||||
def is_in_string(pos: int) -> bool:
|
||||
for s_start, s_end in string_literal_ranges:
|
||||
|
||||
@@ -105,7 +105,7 @@ class TestFindAffectedDashboards:
|
||||
query = virtual_call.kwargs.get("query") or virtual_call.args[0]
|
||||
assert "sql" in query["columns"]
|
||||
assert query["filters"] == [
|
||||
{"col": "sql", "opr": "is_not_null", "value": None}
|
||||
{"col": "sql", "opr": "is_not_null", "value": ""}
|
||||
]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -231,6 +231,43 @@ class TestFindAffectedDashboards:
|
||||
result = await find_affected_dashboards([table], mock_superset)
|
||||
assert 80 in result
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_huge_virtual_dataset_sql_does_not_fail_discovery(self, mock_superset):
|
||||
"""A virtual dataset whose SQL exceeds sqlparse's token limit must not abort the scan.
|
||||
|
||||
The extractor falls back to regex matching for oversized SQL; other datasets
|
||||
are still scanned and matched.
|
||||
"""
|
||||
from src.services.maintenance._dashboard_scanner import find_affected_dashboards
|
||||
huge_sql = (
|
||||
"SELECT " + " + ".join(f"c{i}" for i in range(15000))
|
||||
+ " FROM dm_view.some_other_table"
|
||||
)
|
||||
matching = [
|
||||
{"id": 9, "schema": "", "table_name": "Orders", "sql": "SELECT * FROM raw.orders"}
|
||||
]
|
||||
oversized = [
|
||||
{"id": 8, "schema": "", "table_name": "Huge", "sql": huge_sql}
|
||||
]
|
||||
|
||||
async def side_effect(**kwargs):
|
||||
kind = self._classify_query(kwargs.get("query"))
|
||||
if kind == "virtual_primary":
|
||||
# physical-ish result plus one oversized virtual dataset returned by the
|
||||
# client scan; both must be deduped and the oversized one must not raise.
|
||||
return (2, matching + oversized)
|
||||
return (0, [])
|
||||
|
||||
mock_superset.get_datasets.side_effect = side_effect
|
||||
mock_superset.get_dataset_detail.return_value = {
|
||||
"linked_dashboards": [{"id": 90, "title": "Dashboard 90"}]
|
||||
}
|
||||
|
||||
# The oversized dataset references dm_view.some_other_table; matching raw.orders
|
||||
# must still surface its dashboard without the whole scan raising.
|
||||
result = await find_affected_dashboards(["raw.orders"], mock_superset)
|
||||
assert 90 in result
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_virtual_and_physical_matches_combined(self, mock_superset):
|
||||
"""Physical table matches and virtual SQL matches are both returned."""
|
||||
|
||||
@@ -258,6 +258,22 @@ class TestSqlSpanEdgeCases:
|
||||
result = extract_tables_from_sql_span(sql)
|
||||
assert "raw.sales" in result
|
||||
|
||||
def test_huge_sql_falls_back_to_regex_instead_of_raising(self):
|
||||
"""SQL exceeding sqlparse's token limit (MAX_GROUPING_TOKENS=10000) must not raise.
|
||||
|
||||
Production virtual datasets can be enormous; before this fix a SQLParseError
|
||||
("Maximum number of tokens exceeded (10000)") aborted the whole maintenance scan.
|
||||
It now falls back to regex-only extraction and still finds the target table.
|
||||
"""
|
||||
from src.services.sql_table_extractor import extract_tables_from_sql
|
||||
sql = (
|
||||
"SELECT "
|
||||
+ " + ".join(f"c{i}" for i in range(15000))
|
||||
+ " FROM dm_view.counterparty_td"
|
||||
)
|
||||
result = extract_tables_from_sql(sql)
|
||||
assert "dm_view.counterparty_td" in result
|
||||
|
||||
|
||||
class TestExtractTablesFromJinjaEdge:
|
||||
"""Edge coverage for extract_tables_from_jinja."""
|
||||
|
||||
@@ -52,7 +52,7 @@
|
||||
```
|
||||
Tier 1 (предпочтительно, маленький результат):
|
||||
GET /dataset/?q={"columns":["id","table_name","schema","sql"],
|
||||
"filters":[{"col":"sql","opr":"is_not_null","value":None}]}
|
||||
"filters":[{"col":"sql","opr":"is_not_null","value":""}]}
|
||||
|
||||
Tier 2 (fallback, клиентский скан), если Tier 1 отклонён/упал:
|
||||
GET /dataset/?q={"columns":["id","table_name","schema","sql"]}
|
||||
@@ -85,6 +85,14 @@ Tier 2 (fallback, клиентский скан), если Tier 1 отклонё
|
||||
обнаружение виртуальности строится на `sql`, а не на флаге. Клиентская проверка
|
||||
`is_virtual` в цикле сопоставления уже опирается на непустой `sql`.
|
||||
|
||||
### Примечание по `value` в фильтре
|
||||
|
||||
`value: None` в rison-фильтре невалидно (схема требует number/string/boolean/array),
|
||||
Superset отклоняет его HTTP 400. Для `is_not_null` значение игнорируется оператором,
|
||||
поэтому используется `value: ""` — валидная строка, которую `col.isnot(None)` не
|
||||
использует. Это позволяет Tier 1 работать (маленький серверный фильтр), а не каждый
|
||||
раз откатываться к полному клиентскому скану.
|
||||
|
||||
---
|
||||
|
||||
## Decision 2: AsyncAPIClient.request поднимает не-2xx ответы
|
||||
@@ -103,6 +111,35 @@ Tier 2 (fallback, клиентский скан), если Tier 1 отклонё
|
||||
|
||||
---
|
||||
|
||||
## Decision 3: sqlparse token-limit fallback при извлечении таблиц из SQL
|
||||
|
||||
### Проблема
|
||||
|
||||
После починки обнаружения виртуальных датасетов runtime-сбой сместился в извлечение
|
||||
таблиц. `sql_table_extractor.extract_tables_from_sql_span()` вызывает
|
||||
`sqlparse.parse(sql_text)`; для SQL, у которого больше `MAX_GROUPING_TOKENS = 10000`
|
||||
токенов, sqlparse поднимает `SQLParseError: "Maximum number of tokens exceeded (10000)"`.
|
||||
В `find_affected_dashboards` вызов `extract_tables_from_sql(ds_sql)` в цикле по всем
|
||||
виртуальным датасетам **не был обёрнут в try/except**, поэтому один гигантский
|
||||
виртуальный датасет срывал всё обнаружение (`preview-dashboards` → 502,
|
||||
`start_maintenance` → discovery failed). Это и есть «капа слишком маленькая» из логов.
|
||||
|
||||
### Решение
|
||||
|
||||
`extract_tables_from_sql_span()` оборачивает `sqlparse.parse` + обход токенов в
|
||||
`try/except`. При ошибке (в т.ч. token-limit) список строковых литералов остаётся
|
||||
пустым → `is_in_string()` возвращает False → сохраняются **все** regex-совпадения
|
||||
`schema.table`. Это best-effort сопоставление (допускает возможные false positives из
|
||||
строковых литералов) вместо жёсткого отказа всего скана.
|
||||
|
||||
### Отклонённая альтернатива
|
||||
|
||||
Поднимать `MAX_GROUPING_TOKENS` в `sqlparse` (монакий-патч или правка константы) —
|
||||
хрупко и влияет на производительность парсинга для всех датасетов. Обход только
|
||||
конкретного сбоя безопаснее.
|
||||
|
||||
---
|
||||
|
||||
## Последствия
|
||||
|
||||
1. **Виртуальные датасеты теперь обнаруживаются.** Физические + виртуальные совпадения
|
||||
@@ -119,11 +156,17 @@ Tier 2 (fallback, клиентский скан), если Tier 1 отклонё
|
||||
pagination cap (best-effort: при капе виртуальное совпадение пропускается, физическое
|
||||
сохраняется).
|
||||
|
||||
4. **Проверяемость.** Корневая причина подтверждена по исходникам Superset
|
||||
4. **Гигантские виртуальные датасеты не валят discovery.** `extract_tables_from_sql_span`
|
||||
при token-limit sqlparse (10000) откатывается к regex-only, а не бросает исключение.
|
||||
Один большой виртуальный датасет больше не срывает `preview-dashboards`/`start_maintenance`.
|
||||
Покрыто тестами: `tests/test_sql_table_extractor.py::test_huge_sql_falls_back_to_regex_instead_of_raising`
|
||||
и `tests/services/maintenance/test_dashboard_scanner.py::test_huge_virtual_dataset_sql_does_not_fail_discovery`.
|
||||
|
||||
5. **Проверяемость.** Корневая причина подтверждена по исходникам Superset
|
||||
(`superset/datasets/api.py: search_columns` и `list_columns`; `is_sqllab_view` —
|
||||
модельная колонка в `connectors/sqla/models.py`). Механизм покрыт тестами в
|
||||
`backend/tests/services/maintenance/test_dashboard_scanner.py` (включая end-to-end с
|
||||
реальным `sql_table_extractor` и реальным SQL из продакшн-сценария) и
|
||||
`backend/tests/test_core/test_async_network.py`.
|
||||
модельная колонка в `connectors/sqla/models.py`) и по `sqlparse` (`MAX_GROUPING_TOKENS`).
|
||||
Механизм покрыт тестами в `backend/tests/services/maintenance/test_dashboard_scanner.py`
|
||||
(включая end-to-end с реальным `sql_table_extractor` и реальным SQL из продакшн-сценария),
|
||||
`backend/tests/test_sql_table_extractor.py` и `backend/tests/test_core/test_async_network.py`.
|
||||
|
||||
# [/DEF:Doc.Adr.ADR0020:ADR]
|
||||
|
||||
@@ -37,7 +37,7 @@ ADR фиксирует принятое и проверяемое архитек
|
||||
| [0017](ADR-0017-agent-centric-logging.md) | `ACCEPTED` | Agent-centric traces achieved by editing call sites (meaningful intents, removal of per-request noise) rather than only adding complexity to the logger. |
|
||||
| [0018](ADR-0018-rejected-dataset-review.md) | `REJECTED` | Dataset Review is removed from the active product; `specs/027-dataset-llm-orchestration/` is retained only as archived history. |
|
||||
| [0019](ADR-0019-dashboard-import-mechanism.md) | `ACCEPTED` | UUID-трансформация БД (не strip_databases), cross-filter patching через IdMappingService, password injection flow (await_input→wait_for_input→retry), разделение dry-run/execute, парсинг имён YAML-файлов БД. |
|
||||
| [0020](ADR-0020-maintenance-virtual-dataset-discovery.md) | `ACCEPTED` | Обнаружение виртуальных (SQL) датасетов в maintenance discovery через серверный фильтр `sql is_not_null` (fallback — клиентский скан по непустому `sql`); `AsyncAPIClient.request` поднимает не-2xx ответы (`raise_for_status`), восстанавливая fallback фильтрованный→полный скан. |
|
||||
| [0020](ADR-0020-maintenance-virtual-dataset-discovery.md) | `ACCEPTED` | Обнаружение виртуальных (SQL) датасетов в maintenance discovery через серверный фильтр `sql is_not_null` (fallback — клиентский скан по непустому `sql`); `AsyncAPIClient.request` поднимает не-2xx ответы (`raise_for_status`); sqlparse token-limit (10000) fallback к regex-only для гигантских виртуальных датасетов. |
|
||||
|
||||
## Правила сопровождения
|
||||
|
||||
|
||||
Reference in New Issue
Block a user