diff --git a/.github/demos/check-fix.gif b/.github/demos/check-fix.gif index e0a8abd..1cfa45a 100644 Binary files a/.github/demos/check-fix.gif and b/.github/demos/check-fix.gif differ diff --git a/README.md b/README.md index 845e779..68605fc 100644 --- a/README.md +++ b/README.md @@ -26,31 +26,14 @@ uv tool install sqlclean # or: pip install sqlclean ```

- sqlclean check reports an unused CTE, a temporary table from a custom Rule and a comparison with NULL; sqlclean fix removes the CTE and formats the file; sqlclean fix --unsafe-fixes rewrites = NULL to IS NULL + sqlclean check reports an unused CTE, a case-sensitive email comparison from a custom Rule and a comparison with NULL; sqlclean fix removes the CTE and formats the file; sqlclean fix --unsafe-fixes rewrites = NULL to IS NULL

-`sqlclean check` reports an unused CTE, a temporary table from a custom Rule and a `= NULL` -comparison that is never true. `sqlclean fix` applies the safe fix, removing the CTE, and formats -the file. It leaves `= NULL` alone because rewriting it to `IS NULL` changes which rows match; -`sqlclean fix --unsafe-fixes` applies that fix. The custom Rule's finding has no fix, so it stays. - -## Speed - -Linting the same 10,282 Snowflake SQL files on 4 CPUs, with no cache: - -| Command | Wall time | Time relative to `sqlclean check` | -|---|---:|---:| -| `sqlclean check` (default Rules) | **0.81 s** | 1× | -| `sqlclean check --select SQC` (all Rules) | **1.33 s** | 1.6× | -| `sqlclean check` with three custom Python Rules | **2.27 s** | 2.8× | -| `sqruff lint` | 41.0 s | 51× | -| `sqlfluff lint --processes 4` | 919 s | 1,135× | - -Run again with nothing changed, `sqlclean check` reads its result cache and finishes in 0.23 s. - -The tools don't check the same things, so see the -[benchmarks](https://sqlclean.io/docs/benchmarks/) for coverage, the corpus, the method and how -to run it yourself. +`sqlclean check` reports an unused CTE, a case-sensitive email comparison that a team's custom +Rule forbids, and a `= NULL` comparison that is never true. `sqlclean fix` applies the safe fix, +removing the CTE, and formats the file. It leaves `= NULL` alone because rewriting it to `IS NULL` +changes which rows match; `sqlclean fix --unsafe-fixes` applies that fix. The custom Rule only +reports, so its finding stays. ## Quick start @@ -64,6 +47,30 @@ Add a `sqlclean.toml` to choose a dialect, files or Rules. See the [quickstart](https://sqlclean.io/docs/quickstart/) and [configuration](https://sqlclean.io/docs/configuration/). +## Speed + +Linting the same 10,282 Snowflake SQL files on a Linux machine with 4 CPUs, each tool with its +default rules and no cache: + +| Tool | Wall time | +|---|---:| +| `sqlclean check` | **0.81 s** | +| `sqruff lint` | 41.0 s | +| `sqlfluff lint --processes 4` | 919 s | + +The tools don't check the same things: almost every sqruff and SQLFluff finding comes from a +layout rule, which SQLClean leaves to its formatter. See the +[benchmarks](https://sqlclean.io/docs/benchmarks/) for coverage, the corpus, the method and how +to run it yourself. + +SQLClean in other configurations, on the same files: + +| SQLClean command | Wall time | +|---|---:| +| `sqlclean check --select SQC` (every built-in Rule) | 1.33 s | +| `sqlclean check` plus three custom Python Rules | 2.27 s | +| `sqlclean check` rerun with nothing changed (result cache) | 0.23 s | + ## Correct and safe - **Rules for SQL that runs but is wrong.** Built-in Rules flag comparisons with `NULL`, `NOT IN` @@ -77,30 +84,59 @@ Add a `sqlclean.toml` to choose a dialect, files or Rules. See the ## Custom Rules in Python -A custom Rule is a Python function over the tables, joins, columns and literals of each -statement, already parsed and located by the Rust engine: +A custom Rule is a Python function over each statement, already parsed and located by the Rust +engine. This one enforces a team convention, not a rule of SQL: the columns the team lists in +`EMAIL_COLUMNS` are compared case-insensitively. An `=` or `<>` that reads one of them, bare or as +`LOWER(email)`, passes only when both sides are `LOWER(...)` or a lower-case string literal. It +reads comparisons from the statement's syntax tree, so comments and string literals never match, +and it only reports, because changing a comparison is the author's call. ```python -# sql_rules/naming.py +# sql_rules/team.py +import polyglot_sql as ast # pip install "sqlclean[ast]" + from sqlclean import Finding, RuleContext, Statement, rule +EMAIL_COLUMNS = {"email", "billing_email"} # the columns this team treats as email addresses + + +def email_column(operand: ast.Expression) -> ast.Column | None: + """The designated email column an operand reads, written bare or as LOWER(column).""" + column = operand.this if isinstance(operand, ast.Lower) else operand + is_email = isinstance(column, ast.Column) and column.name.lower() in EMAIL_COLUMNS + return column if is_email else None + + +def lowered(operand: ast.Expression) -> bool: + """Whether an operand is LOWER(...) or a string literal with no upper-case letters.""" + literal = isinstance(operand, ast.Literal) and operand.is_string + return isinstance(operand, ast.Lower) or (literal and operand.name == operand.name.lower()) + @rule( - code="XSQCNAME001", - message="Query reads a temporary table", - remediation="Read from the published table instead.", + code="XSQCTEAM001", + message="Email comparisons must be case-insensitive", + remediation="Compare LOWER(email) with LOWER(...) instead.", enabled_by_default=True, ) -def temporary_table(*, statement: Statement, ctx: RuleContext) -> list[Finding]: +def case_insensitive_email(*, statement: Statement, ctx: RuleContext) -> list[Finding]: + starts = { + column.to_dict()["column"]["span"]["start"] + for comparison in statement.polyglot_ast().find_all(ast.Eq, ast.Neq) + for column in map(email_column, (comparison.this, comparison.expression)) + if column is not None and not all(map(lowered, (comparison.this, comparison.expression))) + } return [ - ctx.finding(subject=statement, node=table) - for table in statement.tables - if table.name.startswith("tmp_") + ctx.finding(subject=statement, node=column) + for column in statement.columns + if column.start - statement.start in starts ] ``` -Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it. See -[custom Rules](https://sqlclean.io/docs/custom-rules/overview/) and +Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it. The syntax tree needs the `ast` +extra: `pip install "sqlclean[ast]"`. See +[custom Rules](https://sqlclean.io/docs/custom-rules/overview/), +[statement facts](https://sqlclean.io/docs/custom-rules/statement-facts/) and [testing custom Rules](https://sqlclean.io/docs/custom-rules/testing/). ## Integrations diff --git a/pyproject.toml b/pyproject.toml index ea597eb..2c789f7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,6 +42,7 @@ packages = ["src/sqlclean"] dev = [ "commitizen>=4.9.1", "fensu>=0.28.0", + "polyglot-sql-chio==0.13.10", # the ast extra, for the README custom Rule test "pre-commit>=4.3.0", "pytest>=8.4.2", "pytest-xdist>=3.8.0", diff --git a/src/sqlclean/rule_testing/_helpers/native_facts.py b/src/sqlclean/rule_testing/_helpers/native_facts.py index 510aedd..fad7500 100644 --- a/src/sqlclean/rule_testing/_helpers/native_facts.py +++ b/src/sqlclean/rule_testing/_helpers/native_facts.py @@ -11,19 +11,18 @@ def native_facts(*, source: str, dialect: str) -> dict[str, object]: - """Return the native statement facts of source, or raise RuleCaseError if it fails.""" + """Return the native facts of source, sent as bytes so newlines stay LF on every platform.""" - completed: subprocess.CompletedProcess[str] = subprocess.run( + completed: subprocess.CompletedProcess[bytes] = subprocess.run( [_executable(), FACTS_COMMAND, "--dialect", dialect], - input=source, + input=source.encode("utf-8"), capture_output=True, - text=True, - encoding="utf-8", check=False, ) if completed.returncode: - raise RuleCaseError(f"sqlclean could not parse the case SQL: {completed.stderr.strip()}") - facts: dict[str, object] = json.loads(completed.stdout) + stderr: str = completed.stderr.decode("utf-8", errors="replace").strip() + raise RuleCaseError(f"sqlclean could not parse the case SQL: {stderr}") + facts: dict[str, object] = json.loads(completed.stdout.decode("utf-8")) return facts diff --git a/tests/integration/src/sqlclean/rule_testing/_test_types.py b/tests/integration/src/sqlclean/rule_testing/_test_types.py index bb99f7e..195a0f2 100644 --- a/tests/integration/src/sqlclean/rule_testing/_test_types.py +++ b/tests/integration/src/sqlclean/rule_testing/_test_types.py @@ -26,3 +26,12 @@ class HarnessFailureTestCase: rule_name: str rule_case: RuleCase expected_error: str + + +@dataclass(frozen=True) +class ReadmeRuleTestCase: + """SQL for the README custom Rule and the text of every column it must report.""" + + description: str + source: str + expected_texts: tuple[str, ...] diff --git a/tests/integration/src/sqlclean/rule_testing/helpers.py b/tests/integration/src/sqlclean/rule_testing/helpers.py index 7a479cd..9659135 100644 --- a/tests/integration/src/sqlclean/rule_testing/helpers.py +++ b/tests/integration/src/sqlclean/rule_testing/helpers.py @@ -103,3 +103,22 @@ def sample_rule(directory: Path, name: str) -> RuleCheck: spec.loader.exec_module(module) check: RuleCheck = getattr(module, name) return check + + +README: Path = Path(__file__).resolve().parents[5] / "README.md" +BLOCK_START: str = "```python\n# sql_rules/team.py\n" + + +def readme_rule(directory: Path) -> RuleCheck: + """Return the README's case_insensitive_email Rule, imported from its code block.""" + + text: str = README.read_text(encoding="utf-8") + start: int = text.index(BLOCK_START) + len(BLOCK_START) + path: Path = directory / "readme_team.py" + path.write_text(text[start : text.index("```", start)], encoding="utf-8") + spec: ModuleSpec | None = importlib.util.spec_from_file_location("readme_team", path) + assert spec is not None and spec.loader is not None + module: ModuleType = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + check: RuleCheck = module.case_insensitive_email + return check diff --git a/tests/integration/src/sqlclean/rule_testing/test_evaluate_rule.py b/tests/integration/src/sqlclean/rule_testing/test_evaluate_rule.py index d04a50c..5060085 100644 --- a/tests/integration/src/sqlclean/rule_testing/test_evaluate_rule.py +++ b/tests/integration/src/sqlclean/rule_testing/test_evaluate_rule.py @@ -51,6 +51,17 @@ expected_texts=("1",), expected_fixed_source="SELECT 'é' AS mark, COUNT(*) AS n FROM t", ), + HarnessTestCase( + description="a finding after line breaks keeps its offset on every platform", + rule_name="forbidden_prefix", + rule_case=RuleCase( + description="multi-line read", + source="-- note\nSELECT id\nFROM tmp_orders", + expected_finding_count=1, + ), + expected_texts=("tmp_orders",), + expected_fixed_source="-- note\nSELECT id\nFROM tmp_orders", + ), HarnessTestCase( description="a file Rule sees every statement of the file", rule_name="one_statement", diff --git a/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py b/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py new file mode 100644 index 0000000..1b2c950 --- /dev/null +++ b/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py @@ -0,0 +1,87 @@ +"""The custom Rule example in README.md, run as written through the Rule harness.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from sqlclean.rules.types import RuleCheck +from sqlclean.testing import RuleCase, RuleResult, evaluate_rule +from tests.integration.src.sqlclean.rule_testing._test_types import ReadmeRuleTestCase +from tests.integration.src.sqlclean.rule_testing.helpers import readme_rule + + +@pytest.mark.parametrize( + "test_case", + [ + ReadmeRuleTestCase( + description="a bare email column compared with a mixed-case literal", + source="SELECT id FROM customers c WHERE c.email = 'Alice@example.com'", + expected_texts=("c.email",), + ), + ReadmeRuleTestCase( + description="LOWER on one side only still compares with a mixed-case literal", + source="SELECT id FROM customers WHERE LOWER(email) = 'Alice@Example.com'", + expected_texts=("email",), + ), + ReadmeRuleTestCase( + description="a designated column on the right of <> in upper case", + source="SELECT id FROM customers WHERE 'Carol@example.com' <> BILLING_EMAIL", + expected_texts=("BILLING_EMAIL",), + ), + ReadmeRuleTestCase( + description="UPPER does not normalize the other side to lower case", + source="SELECT id FROM customers WHERE LOWER(email) = UPPER('x')", + expected_texts=("email",), + ), + ReadmeRuleTestCase( + description="a bare column against a lower-case literal is still not normalized", + source="SELECT id FROM customers WHERE email = 'carol@example.com'", + expected_texts=("email",), + ), + ReadmeRuleTestCase( + description="a comparison inside a CTE after a multibyte literal", + source="WITH u AS (SELECT 'é' AS m, email FROM users WHERE email = 'X@y.com') " + "SELECT * FROM u", + expected_texts=("email",), + ), + ReadmeRuleTestCase( + description="LOWER on both sides passes", + source="SELECT * FROM u JOIN a ON LOWER(u.email) = LOWER(a.email)", + expected_texts=(), + ), + ReadmeRuleTestCase( + description="LOWER against a lower-case literal passes", + source="SELECT id FROM customers WHERE LOWER(email) = 'bob@example.com'", + expected_texts=(), + ), + ReadmeRuleTestCase( + description="comments, strings, other columns, LIKE and projections never match", + source="-- email = 'A@b.c'\nSELECT email, 'email = x' AS note FROM customers " + "WHERE email_verified = TRUE AND email LIKE '%dave%'", + expected_texts=(), + ), + ], + ids=lambda case: case.description, +) +def test_given_readme_email_rule_when_evaluating_then_unnormalized_comparisons_are_reported( + tmp_path: Path, test_case: ReadmeRuleTestCase +) -> None: + rule: RuleCheck = readme_rule(tmp_path) + + result: RuleResult = evaluate_rule( + rule=rule, + test_case=RuleCase( + description=test_case.description, + source=test_case.source, + expected_finding_count=len(test_case.expected_texts), + dialect="duckdb", + ), + ) + texts: tuple[str, ...] = tuple( + test_case.source[finding.start : finding.end] for finding in result.findings + ) + + assert texts == test_case.expected_texts + assert result.fixed_source == test_case.source diff --git a/uv.lock b/uv.lock index 765ac06..73762bd 100644 --- a/uv.lock +++ b/uv.lock @@ -630,6 +630,7 @@ ast = [ dev = [ { name = "commitizen" }, { name = "fensu" }, + { name = "polyglot-sql-chio" }, { name = "pre-commit" }, { name = "pytest" }, { name = "pytest-xdist" }, @@ -648,6 +649,7 @@ provides-extras = ["ast"] dev = [ { name = "commitizen", specifier = ">=4.9.1" }, { name = "fensu", specifier = ">=0.28.0" }, + { name = "polyglot-sql-chio", specifier = "==0.13.10" }, { name = "pre-commit", specifier = ">=4.3.0" }, { name = "pytest", specifier = ">=8.4.2" }, { name = "pytest-xdist", specifier = ">=3.8.0" }, diff --git a/website/recordings/README.md b/website/recordings/README.md index d4af8a9..c80b098 100644 --- a/website/recordings/README.md +++ b/website/recordings/README.md @@ -16,6 +16,8 @@ The README uses `check-fix`, copied into `.github/demos/`. - DejaVu Sans Mono, the font the tapes use. - `sqlclean` on `PATH` with its Python package in the same environment, or `SQLCLEAN_BIN` pointing at a directory that contains the `sqlclean` to record. +- The `ast` extra (`pip install "sqlclean[ast]"`) for the `check-fix` custom Rule, or + `SQLCLEAN_PYTHON` pointing at an interpreter that has it. ## Record diff --git a/website/recordings/prepare.sh b/website/recordings/prepare.sh index 00621e6..9d50da1 100755 --- a/website/recordings/prepare.sh +++ b/website/recordings/prepare.sh @@ -9,7 +9,7 @@ mkdir -p "$dir" cd "$dir" case "$demo" in check-fix) - # One query with a NULL comparison, an unused CTE and a temporary table that a custom Rule forbids. + # One query with an unused CTE, a case-sensitive email comparison that a custom Rule forbids, and a NULL comparison. mkdir -p models sql_rules cat > sqlclean.toml <<'TOML' dialect = "duckdb" @@ -23,24 +23,48 @@ with refunds as ( ) select o.order_id, c.name, o.total from orders o -join tmp_customers c on o.customer_id = c.customer_id -where o.shipped_at = null +join customers c on o.customer_id = c.customer_id +where c.email = 'Alice@example.com' + and o.shipped_at = null SQL - cat > sql_rules/naming.py <<'PY' + cat > sql_rules/team.py <<'PY' +import polyglot_sql as ast # pip install "sqlclean[ast]" + from sqlclean import Finding, RuleContext, Statement, rule +EMAIL_COLUMNS = {"email", "billing_email"} # the columns this team treats as email addresses + + +def email_column(operand: ast.Expression) -> ast.Column | None: + """The designated email column an operand reads, written bare or as LOWER(column).""" + column = operand.this if isinstance(operand, ast.Lower) else operand + is_email = isinstance(column, ast.Column) and column.name.lower() in EMAIL_COLUMNS + return column if is_email else None + + +def lowered(operand: ast.Expression) -> bool: + """Whether an operand is LOWER(...) or a string literal with no upper-case letters.""" + literal = isinstance(operand, ast.Literal) and operand.is_string + return isinstance(operand, ast.Lower) or (literal and operand.name == operand.name.lower()) + @rule( - code="XSQCNAME001", - message="Query reads a temporary table", - remediation="Read from the published table instead.", + code="XSQCTEAM001", + message="Email comparisons must be case-insensitive", + remediation="Compare LOWER(email) with LOWER(...) instead.", enabled_by_default=True, ) -def temporary_table(*, statement: Statement, ctx: RuleContext) -> list[Finding]: +def case_insensitive_email(*, statement: Statement, ctx: RuleContext) -> list[Finding]: + starts = { + column.to_dict()["column"]["span"]["start"] + for comparison in statement.polyglot_ast().find_all(ast.Eq, ast.Neq) + for column in map(email_column, (comparison.this, comparison.expression)) + if column is not None and not all(map(lowered, (comparison.this, comparison.expression))) + } return [ - ctx.finding(subject=statement, node=table) - for table in statement.tables - if table.name.startswith("tmp_") + ctx.finding(subject=statement, node=column) + for column in statement.columns + if column.start - statement.start in starts ] PY ;;