diff --git a/README.md b/README.md index e2357d9..1fd0e62 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ question ─▶ schema retrieval ─▶ LLM: plan + SQL ─▶ AST validation | Validation | deterministic | `sqlglot` AST checks, row limit enforced (see below). The SQL that runs is regenerated from the validated tree. | | Execution | deterministic | Read-only connection, statement timeout, client-side row cap. | | Repair | LLM, bounded | A repairable failure (validator rejection, SQL error, timeout, bad result, malformed model reply) is sent back to the model at most `MAX_RETRIES` times; every repair is validated again. Unsafe intent is never retried. | -| Result checks | deterministic | Empty result, aggregate over nothing, ranking led by NULL, NULL-only column, row limit reached, duplicates. Missing-value probes catch filter values and `LIKE` patterns that match nothing (e.g. `'Ivory Coast'`, a time written `'0:01:54%'` where `'1:54.123'` is stored) and show how stored values look. | +| Result checks | deterministic | Empty result, aggregate over nothing, ranking led by NULL, columns concatenated into one, NULL-only column, row limit reached, duplicates. Missing-value probes catch filter values and `LIKE` patterns that match nothing (e.g. `'Ivory Coast'`, a time written `'0:01:54%'` where `'1:54.123'` is stored) and show how stored values look. | | Chart | deterministic | Chosen from column kinds and values: stat tile, line, bar, scatter or none. The model's suggestion is only a tie-breaker. | | Answer | LLM + check | 1–3 sentences. Every number must come from the rows; otherwise a template answer built from the rows is used. | diff --git a/backend/app/agent/controller.py b/backend/app/agent/controller.py index fa27ce7..8c18f74 100644 --- a/backend/app/agent/controller.py +++ b/backend/app/agent/controller.py @@ -432,6 +432,7 @@ def done(outcome: AgentResult, **detail: Any) -> _Solved: tables=validated.tables, limit=validated.limit, limit_action=validated.limit_action, + float_casts=validated.float_casts, plan_warnings=( check_plan(plan, validated.tables, validated.limit) if plan else ["no plan returned"] ), diff --git a/backend/app/agent/result_checks.py b/backend/app/agent/result_checks.py index b0dc44c..eb88f05 100644 --- a/backend/app/agent/result_checks.py +++ b/backend/app/agent/result_checks.py @@ -102,6 +102,20 @@ def check_result( ) ) + if isinstance(tree, exp.Select) and len(tree.expressions) == len(result.columns): + for column, projection in zip(result.columns, tree.expressions, strict=True): + joined = _joined_columns(projection) + if joined: + checks.append( + ResultCheck( + code="joined_columns", + severity="warning", + message=f"{column} joins {_listing(joined)} into one text value.", + column=column, + repairable=True, + ) + ) + if result.row_count > 1: for index, column in enumerate(result.columns): if all(row[index] is None for row in result.rows): @@ -139,6 +153,23 @@ def _hashable(value: Any) -> Any: return repr(value) if isinstance(value, (list, dict)) else value +def _joined_columns(projection: exp.Expr) -> list[str]: + """Names of the stored columns an output column concatenates (`a || ' ' || b`, CONCAT, + CONCAT_WS), if it joins two or more; otherwise empty. A single column with a label + ('$' || price) is a format, and a concatenation inside an aggregate (COUNT(DISTINCT a || b)) + or a subquery is a computation, not an output: neither is reported.""" + for node in projection.walk(prune=lambda n: isinstance(n, (exp.AggFunc, exp.Subquery))): + if isinstance(node, (exp.DPipe, exp.Concat, exp.ConcatWs)): + names = dict.fromkeys((c.table, c.name) for c in node.find_all(exp.Column, bfs=False)) + if len(names) > 1: + return [name for _, name in names] + return [] + + +def _listing(names: list[str]) -> str: + return ", ".join(names[:-1]) + f" and {names[-1]}" + + def _string_filters(tree: exp.Expr) -> Iterator[tuple[exp.Table, str, str, exp.Literal]]: """(table, column, operator, literal) for each `column = 'text'`, `column IN ('text', ...)` and `column [I]LIKE 'pattern'` filter on a real table column, in WHERE and JOIN conditions of diff --git a/backend/app/agent/sql_generator.py b/backend/app/agent/sql_generator.py index c982afd..92aea6f 100644 --- a/backend/app/agent/sql_generator.py +++ b/backend/app/agent/sql_generator.py @@ -17,7 +17,7 @@ from app.agent.schema_retriever import SchemaContext from app.llm.client import LLMCall, LLMClient -PROMPT_VERSION = "sql-generator/7" +PROMPT_VERSION = "sql-generator/9" MAX_HISTORY_TURNS = 3 SYSTEM_PROMPT = """\ @@ -168,7 +168,7 @@ def generate_sql( return llm.complete_json(system, prompt, GeneratedSQL, temperature) -REPAIR_PROMPT_VERSION = "sql-repair/3" +REPAIR_PROMPT_VERSION = "sql-repair/4" _REPAIR_HINTS = { "timeout": ( @@ -186,6 +186,7 @@ def generate_sql( "(see the listed values and column comments), or match it case-insensitively." ), "null_first_in_ranking": "The ranking starts with NULL values: exclude NULLs or use NULLS LAST.", + "joined_columns": "Return each stored column as its own result column instead of concatenating them.", } diff --git a/backend/app/agent/sql_validator.py b/backend/app/agent/sql_validator.py index b33d9bb..54c2240 100644 --- a/backend/app/agent/sql_validator.py +++ b/backend/app/agent/sql_validator.py @@ -86,6 +86,7 @@ class ValidatedSQL: tables: list[str] # qualified names of the database tables read limit: int limit_action: Literal["kept", "added", "clamped"] + float_casts: int = 0 # NUMERIC casts in a division computed as DOUBLE PRECISION instead def _reject(code: RejectionCode, message: str) -> SQLRejectedError: @@ -259,6 +260,31 @@ def _enforce_limit(tree: exp.Query, max_rows: int) -> tuple[int, Literal["kept", return max_rows, "clamped" +# Arithmetic between a cast and the division it feeds: CAST(x AS NUMERIC) * 100 / total. +_ARITHMETIC = (exp.Paren, exp.Mul, exp.Add, exp.Sub, exp.Neg) + + +def _float_divisions(tree: exp.Expr) -> int: + """Compute ratios in floating point: CAST(x AS NUMERIC) without precision, where it only serves + a division, becomes CAST(x AS DOUBLE PRECISION). Models cast to NUMERIC to avoid integer + division; the result then comes back as an arbitrary-precision decimal where a ratio or a + percentage is expected to be a float (BIRD's answers are, and an exact comparison of + 0.0657... as decimal and as float fails). A cast with precision (NUMERIC(10, 2)) is a + deliberate choice and stays, and so does any cast that is not part of a division.""" + rewritten = 0 + for cast in list(tree.find_all(exp.Cast)): + to = cast.args.get("to") + if not isinstance(to, exp.DataType) or to.this != exp.DataType.Type.DECIMAL or to.expressions: + continue + node: exp.Expr = cast + while isinstance(node.parent, _ARITHMETIC): + node = node.parent + if isinstance(node.parent, exp.Div): + cast.set("to", exp.DataType.build("DOUBLE PRECISION")) + rewritten += 1 + return rewritten + + def validate_sql(sql: str, profile: DatabaseProfile, dialect: str, max_rows: int) -> ValidatedSQL: """Accept or reject generated SQL. Raises SQLRejectedError with a machine-readable code.""" tree = _parse(sql, dialect) @@ -267,4 +293,7 @@ def validate_sql(sql: str, profile: DatabaseProfile, dialect: str, max_rows: int tables = _check_tables(tree, profile) _check_columns(tree, profile, dialect) limit, action = _enforce_limit(tree, max_rows) - return ValidatedSQL(sql=tree.sql(dialect=dialect), tables=tables, limit=limit, limit_action=action) + casts = _float_divisions(tree) + return ValidatedSQL( + sql=tree.sql(dialect=dialect), tables=tables, limit=limit, limit_action=action, float_casts=casts + ) diff --git a/backend/tests/test_agent.py b/backend/tests/test_agent.py index 89bbef8..451379f 100644 --- a/backend/tests/test_agent.py +++ b/backend/tests/test_agent.py @@ -514,6 +514,17 @@ def test_ranking_led_by_nulls_is_repaired(shop) -> None: assert "null_first_in_ranking" in llm.calls[1][1] and "NULLS LAST" in llm.calls[1][1] +def test_concatenated_columns_are_repaired_into_separate_columns(shop) -> None: + llm = sequence( + reply("SELECT id, status || ' / ' || ordered_at AS summary FROM shop.orders WHERE id = 1"), + reply("SELECT id, status, ordered_at FROM shop.orders WHERE id = 1"), + ) + result = AgentController(llm, max_rows=100).run("Status and date of order 1?", shop) + assert result.status == "success" and result.columns == ["id", "status", "ordered_at"] + assert "summary joins status and ordered_at into one text value" in llm.calls[1][1] + assert result.checks == [] and result.metadata.retry_count == 1 + + def test_earlier_result_is_kept_when_a_repair_ends_worse(shop) -> None: llm = sequence( reply("SELECT count(*) AS n FROM shop.orders WHERE status = 'Shipped'"), diff --git a/backend/tests/test_result_checks.py b/backend/tests/test_result_checks.py index 17e0224..25ada5d 100644 --- a/backend/tests/test_result_checks.py +++ b/backend/tests/test_result_checks.py @@ -77,6 +77,42 @@ def test_duplicate_rows() -> None: assert codes(result(["a"], [[1], [1], [2]])) == ["duplicate_rows"] +@pytest.mark.parametrize( + "projection", + [ + "m.first_name || ' ' || m.last_name AS full_name", + "CONCAT(first_name, ' ', last_name) AS full_name", + "CONCAT_WS(' ', first_name, last_name) AS full_name", + "TRIM(first_name || ' ' || COALESCE(last_name, '')) AS full_name", + ], +) +def test_concatenated_columns_are_repairable(projection: str) -> None: + res = result(["id", "full_name"], [[1, "Ada Lovelace"]]) + checks = check_result(res, f"SELECT id, {projection} FROM member AS m", "postgres", 100, "kept") + assert [(c.code, c.column, c.repairable) for c in checks] == [("joined_columns", "full_name", True)] + assert checks[0].message == "full_name joins first_name and last_name into one text value." + + +def test_concatenated_columns_in_the_final_select_of_a_cte_query() -> None: + sql = "WITH m AS (SELECT * FROM member) SELECT city || ', ' || country AS place FROM m" + checks = check_result(result(["place"], [["Oslo, Norway"]]), sql, "postgres", 100, "kept") + assert [c.message for c in checks] == ["place joins city and country into one text value."] + + +@pytest.mark.parametrize( + "sql", + [ + "SELECT '$' || price AS price FROM t", # one column with a label: a format + "SELECT COUNT(DISTINCT a || b) AS pairs FROM t", # computed over, not returned + "SELECT STRING_AGG(first_name || ' ' || last_name, ', ') AS names FROM t", + "SELECT (SELECT a || b FROM u LIMIT 1) AS x FROM t", # inside a subquery + "WITH k AS (SELECT a || '-' || b AS key FROM t) SELECT key FROM k", # a key built in a CTE + ], +) +def test_other_concatenation_is_not_reported(sql: str) -> None: + assert codes(result(["x"], [["v"]]), sql) == [] + + # --- missing-value probes (integration) -------------------------------------------------- diff --git a/backend/tests/test_sql_generator.py b/backend/tests/test_sql_generator.py index 369f3ed..4b0a3f4 100644 --- a/backend/tests/test_sql_generator.py +++ b/backend/tests/test_sql_generator.py @@ -120,6 +120,17 @@ def test_repair_prompt_contains_the_failure_and_a_hint() -> None: assert "Do not repeat the same SQL" in prompt +def test_joined_columns_repair_asks_for_separate_columns() -> None: + from app.agent.schema_retriever import SchemaContext + from app.agent.sql_generator import FailedAttempt, build_repair_prompt + + message = "full_name joins first_name and last_name into one text value." + failed = FailedAttempt("SELECT a || b FROM t", "result", "joined_columns", message) + prompt = build_repair_prompt("q", SchemaContext(tables=[], text=""), failed) + assert f"result failed a check (joined_columns): {message}" in prompt + assert "its own result column" in prompt + + def test_validation_failure_is_described_as_a_rejection() -> None: from app.agent.schema_retriever import SchemaContext from app.agent.sql_generator import FailedAttempt, build_repair_prompt diff --git a/backend/tests/test_sql_validator.py b/backend/tests/test_sql_validator.py index a353a3b..9b946e2 100644 --- a/backend/tests/test_sql_validator.py +++ b/backend/tests/test_sql_validator.py @@ -242,6 +242,38 @@ def test_limit_applies_to_the_whole_union() -> None: assert result.limit_action == "added" +# --- ratios in floating point ----------------------------------------------------------- + + +@pytest.mark.parametrize( + "expression", + [ + "CAST(COUNT(*) AS NUMERIC) / COUNT(*)", + "CAST(COUNT(*) AS DECIMAL) * 100 / NULLIF(COUNT(*), 0)", + "(CAST(COUNT(*) AS DECIMAL)) / COUNT(*)", + "COUNT(*) / COUNT(*)::numeric", + ], +) +def test_numeric_casts_in_a_division_become_double_precision(expression: str) -> None: + result = validate(f"SELECT {expression} AS share FROM countries") + assert result.float_casts == 1 + assert "DOUBLE PRECISION" in result.sql and "DECIMAL" not in result.sql + validate(result.sql) # what runs is still a valid query + + +@pytest.mark.parametrize( + "expression", + [ + "CAST(COUNT(*) AS NUMERIC(10, 2)) / COUNT(*)", # a chosen precision stays + "CAST(COUNT(*) AS NUMERIC)", # no division + "SUM(CAST(LENGTH(name) AS NUMERIC)) / COUNT(*)", # inside an aggregate: not the ratio itself + ], +) +def test_other_numeric_casts_are_left_alone(expression: str) -> None: + result = validate(f"SELECT {expression} AS value FROM countries") + assert result.float_casts == 0 and "DOUBLE PRECISION" not in result.sql + + def test_inner_limits_do_not_count_as_the_outer_limit() -> None: result = validate("SELECT name FROM countries WHERE id IN (SELECT country_id FROM co2_emissions LIMIT 5)") assert result.limit_action == "added"