Skip to content

Commit e66e5f5

Browse files
authored
fix(testing): keep Rule case offsets on Windows, and show an email Rule in the README (#12)
* docs: show a case-insensitive email Rule in the README example and demo Co-authored-by: Kevin Longe <kvlonge@gmail.com> * docs: require normalized operands in the README email Rule, test it, and split the speed table The README Rule now flags an email comparison unless both sides are LOWER(...) or a lower-case string literal, so LOWER(email) compared with a mixed-case literal is reported. A test runs the README's code block through evaluate_rule. The speed table separates the competitor comparison from SQLClean's other configurations and drops the ratio column. Co-authored-by: Kevin Longe <kvlonge@gmail.com> * docs: move the README quick start before the speed tables Co-authored-by: Kevin Longe <kvlonge@gmail.com> * fix(testing): keep Rule case offsets right on Windows evaluate_rule piped the case SQL to the native executable in text mode, which on Windows turned each newline into CRLF, so statement text and finding offsets drifted by one character per line. Send UTF-8 bytes. Co-authored-by: Kevin Longe <kvlonge@gmail.com> ---------
1 parent 5587732 commit e66e5f5

11 files changed

Lines changed: 243 additions & 53 deletions

File tree

‎.github/demos/check-fix.gif‎

28 KB
Loading

‎README.md‎

Lines changed: 71 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -26,31 +26,14 @@ uv tool install sqlclean # or: pip install sqlclean
2626
```
2727

2828
<p align="center">
29-
<img src="https://raw.githubusercontent.com/sqlstack-io/sqlclean/main/.github/demos/check-fix.gif" alt="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" width="100%">
29+
<img src="https://raw.githubusercontent.com/sqlstack-io/sqlclean/main/.github/demos/check-fix.gif" alt="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" width="100%">
3030
</p>
3131

32-
`sqlclean check` reports an unused CTE, a temporary table from a custom Rule and a `= NULL`
33-
comparison that is never true. `sqlclean fix` applies the safe fix, removing the CTE, and formats
34-
the file. It leaves `= NULL` alone because rewriting it to `IS NULL` changes which rows match;
35-
`sqlclean fix --unsafe-fixes` applies that fix. The custom Rule's finding has no fix, so it stays.
36-
37-
## Speed
38-
39-
Linting the same 10,282 Snowflake SQL files on 4 CPUs, with no cache:
40-
41-
| Command | Wall time | Time relative to `sqlclean check` |
42-
|---|---:|---:|
43-
| `sqlclean check` (default Rules) | **0.81 s** | 1× |
44-
| `sqlclean check --select SQC` (all Rules) | **1.33 s** | 1.6× |
45-
| `sqlclean check` with three custom Python Rules | **2.27 s** | 2.8× |
46-
| `sqruff lint` | 41.0 s | 51× |
47-
| `sqlfluff lint --processes 4` | 919 s | 1,135× |
48-
49-
Run again with nothing changed, `sqlclean check` reads its result cache and finishes in 0.23 s.
50-
51-
The tools don't check the same things, so see the
52-
[benchmarks](https://sqlclean.io/docs/benchmarks/) for coverage, the corpus, the method and how
53-
to run it yourself.
32+
`sqlclean check` reports an unused CTE, a case-sensitive email comparison that a team's custom
33+
Rule forbids, and a `= NULL` comparison that is never true. `sqlclean fix` applies the safe fix,
34+
removing the CTE, and formats the file. It leaves `= NULL` alone because rewriting it to `IS NULL`
35+
changes which rows match; `sqlclean fix --unsafe-fixes` applies that fix. The custom Rule only
36+
reports, so its finding stays.
5437

5538
## Quick start
5639

@@ -64,6 +47,30 @@ Add a `sqlclean.toml` to choose a dialect, files or Rules. See the
6447
[quickstart](https://sqlclean.io/docs/quickstart/) and
6548
[configuration](https://sqlclean.io/docs/configuration/).
6649

50+
## Speed
51+
52+
Linting the same 10,282 Snowflake SQL files on a Linux machine with 4 CPUs, each tool with its
53+
default rules and no cache:
54+
55+
| Tool | Wall time |
56+
|---|---:|
57+
| `sqlclean check` | **0.81 s** |
58+
| `sqruff lint` | 41.0 s |
59+
| `sqlfluff lint --processes 4` | 919 s |
60+
61+
The tools don't check the same things: almost every sqruff and SQLFluff finding comes from a
62+
layout rule, which SQLClean leaves to its formatter. See the
63+
[benchmarks](https://sqlclean.io/docs/benchmarks/) for coverage, the corpus, the method and how
64+
to run it yourself.
65+
66+
SQLClean in other configurations, on the same files:
67+
68+
| SQLClean command | Wall time |
69+
|---|---:|
70+
| `sqlclean check --select SQC` (every built-in Rule) | 1.33 s |
71+
| `sqlclean check` plus three custom Python Rules | 2.27 s |
72+
| `sqlclean check` rerun with nothing changed (result cache) | 0.23 s |
73+
6774
## Correct and safe
6875

6976
- **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
7784

7885
## Custom Rules in Python
7986

80-
A custom Rule is a Python function over the tables, joins, columns and literals of each
81-
statement, already parsed and located by the Rust engine:
87+
A custom Rule is a Python function over each statement, already parsed and located by the Rust
88+
engine. This one enforces a team convention, not a rule of SQL: the columns the team lists in
89+
`EMAIL_COLUMNS` are compared case-insensitively. An `=` or `<>` that reads one of them, bare or as
90+
`LOWER(email)`, passes only when both sides are `LOWER(...)` or a lower-case string literal. It
91+
reads comparisons from the statement's syntax tree, so comments and string literals never match,
92+
and it only reports, because changing a comparison is the author's call.
8293

8394
```python
84-
# sql_rules/naming.py
95+
# sql_rules/team.py
96+
import polyglot_sql as ast # pip install "sqlclean[ast]"
97+
8598
from sqlclean import Finding, RuleContext, Statement, rule
8699

100+
EMAIL_COLUMNS = {"email", "billing_email"} # the columns this team treats as email addresses
101+
102+
103+
def email_column(operand: ast.Expression) -> ast.Column | None:
104+
"""The designated email column an operand reads, written bare or as LOWER(column)."""
105+
column = operand.this if isinstance(operand, ast.Lower) else operand
106+
is_email = isinstance(column, ast.Column) and column.name.lower() in EMAIL_COLUMNS
107+
return column if is_email else None
108+
109+
110+
def lowered(operand: ast.Expression) -> bool:
111+
"""Whether an operand is LOWER(...) or a string literal with no upper-case letters."""
112+
literal = isinstance(operand, ast.Literal) and operand.is_string
113+
return isinstance(operand, ast.Lower) or (literal and operand.name == operand.name.lower())
114+
87115

88116
@rule(
89-
code="XSQCNAME001",
90-
message="Query reads a temporary table",
91-
remediation="Read from the published table instead.",
117+
code="XSQCTEAM001",
118+
message="Email comparisons must be case-insensitive",
119+
remediation="Compare LOWER(email) with LOWER(...) instead.",
92120
enabled_by_default=True,
93121
)
94-
def temporary_table(*, statement: Statement, ctx: RuleContext) -> list[Finding]:
122+
def case_insensitive_email(*, statement: Statement, ctx: RuleContext) -> list[Finding]:
123+
starts = {
124+
column.to_dict()["column"]["span"]["start"]
125+
for comparison in statement.polyglot_ast().find_all(ast.Eq, ast.Neq)
126+
for column in map(email_column, (comparison.this, comparison.expression))
127+
if column is not None and not all(map(lowered, (comparison.this, comparison.expression)))
128+
}
95129
return [
96-
ctx.finding(subject=statement, node=table)
97-
for table in statement.tables
98-
if table.name.startswith("tmp_")
130+
ctx.finding(subject=statement, node=column)
131+
for column in statement.columns
132+
if column.start - statement.start in starts
99133
]
100134
```
101135

102-
Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it. See
103-
[custom Rules](https://sqlclean.io/docs/custom-rules/overview/) and
136+
Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it. The syntax tree needs the `ast`
137+
extra: `pip install "sqlclean[ast]"`. See
138+
[custom Rules](https://sqlclean.io/docs/custom-rules/overview/),
139+
[statement facts](https://sqlclean.io/docs/custom-rules/statement-facts/) and
104140
[testing custom Rules](https://sqlclean.io/docs/custom-rules/testing/).
105141

106142
## Integrations

‎pyproject.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ packages = ["src/sqlclean"]
4242
dev = [
4343
"commitizen>=4.9.1",
4444
"fensu>=0.28.0",
45+
"polyglot-sql-chio==0.13.10", # the ast extra, for the README custom Rule test
4546
"pre-commit>=4.3.0",
4647
"pytest>=8.4.2",
4748
"pytest-xdist>=3.8.0",

‎src/sqlclean/rule_testing/_helpers/native_facts.py‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,18 @@
1111

1212

1313
def native_facts(*, source: str, dialect: str) -> dict[str, object]:
14-
"""Return the native statement facts of source, or raise RuleCaseError if it fails."""
14+
"""Return the native facts of source, sent as bytes so newlines stay LF on every platform."""
1515

16-
completed: subprocess.CompletedProcess[str] = subprocess.run(
16+
completed: subprocess.CompletedProcess[bytes] = subprocess.run(
1717
[_executable(), FACTS_COMMAND, "--dialect", dialect],
18-
input=source,
18+
input=source.encode("utf-8"),
1919
capture_output=True,
20-
text=True,
21-
encoding="utf-8",
2220
check=False,
2321
)
2422
if completed.returncode:
25-
raise RuleCaseError(f"sqlclean could not parse the case SQL: {completed.stderr.strip()}")
26-
facts: dict[str, object] = json.loads(completed.stdout)
23+
stderr: str = completed.stderr.decode("utf-8", errors="replace").strip()
24+
raise RuleCaseError(f"sqlclean could not parse the case SQL: {stderr}")
25+
facts: dict[str, object] = json.loads(completed.stdout.decode("utf-8"))
2726
return facts
2827

2928

‎tests/integration/src/sqlclean/rule_testing/_test_types.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,12 @@ class HarnessFailureTestCase:
2626
rule_name: str
2727
rule_case: RuleCase
2828
expected_error: str
29+
30+
31+
@dataclass(frozen=True)
32+
class ReadmeRuleTestCase:
33+
"""SQL for the README custom Rule and the text of every column it must report."""
34+
35+
description: str
36+
source: str
37+
expected_texts: tuple[str, ...]

‎tests/integration/src/sqlclean/rule_testing/helpers.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,22 @@ def sample_rule(directory: Path, name: str) -> RuleCheck:
103103
spec.loader.exec_module(module)
104104
check: RuleCheck = getattr(module, name)
105105
return check
106+
107+
108+
README: Path = Path(__file__).resolve().parents[5] / "README.md"
109+
BLOCK_START: str = "```python\n# sql_rules/team.py\n"
110+
111+
112+
def readme_rule(directory: Path) -> RuleCheck:
113+
"""Return the README's case_insensitive_email Rule, imported from its code block."""
114+
115+
text: str = README.read_text(encoding="utf-8")
116+
start: int = text.index(BLOCK_START) + len(BLOCK_START)
117+
path: Path = directory / "readme_team.py"
118+
path.write_text(text[start : text.index("```", start)], encoding="utf-8")
119+
spec: ModuleSpec | None = importlib.util.spec_from_file_location("readme_team", path)
120+
assert spec is not None and spec.loader is not None
121+
module: ModuleType = importlib.util.module_from_spec(spec)
122+
spec.loader.exec_module(module)
123+
check: RuleCheck = module.case_insensitive_email
124+
return check

‎tests/integration/src/sqlclean/rule_testing/test_evaluate_rule.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,17 @@
5151
expected_texts=("1",),
5252
expected_fixed_source="SELECT 'é' AS mark, COUNT(*) AS n FROM t",
5353
),
54+
HarnessTestCase(
55+
description="a finding after line breaks keeps its offset on every platform",
56+
rule_name="forbidden_prefix",
57+
rule_case=RuleCase(
58+
description="multi-line read",
59+
source="-- note\nSELECT id\nFROM tmp_orders",
60+
expected_finding_count=1,
61+
),
62+
expected_texts=("tmp_orders",),
63+
expected_fixed_source="-- note\nSELECT id\nFROM tmp_orders",
64+
),
5465
HarnessTestCase(
5566
description="a file Rule sees every statement of the file",
5667
rule_name="one_statement",
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
"""The custom Rule example in README.md, run as written through the Rule harness."""
2+
3+
from __future__ import annotations
4+
5+
from pathlib import Path
6+
7+
import pytest
8+
9+
from sqlclean.rules.types import RuleCheck
10+
from sqlclean.testing import RuleCase, RuleResult, evaluate_rule
11+
from tests.integration.src.sqlclean.rule_testing._test_types import ReadmeRuleTestCase
12+
from tests.integration.src.sqlclean.rule_testing.helpers import readme_rule
13+
14+
15+
@pytest.mark.parametrize(
16+
"test_case",
17+
[
18+
ReadmeRuleTestCase(
19+
description="a bare email column compared with a mixed-case literal",
20+
source="SELECT id FROM customers c WHERE c.email = 'Alice@example.com'",
21+
expected_texts=("c.email",),
22+
),
23+
ReadmeRuleTestCase(
24+
description="LOWER on one side only still compares with a mixed-case literal",
25+
source="SELECT id FROM customers WHERE LOWER(email) = 'Alice@Example.com'",
26+
expected_texts=("email",),
27+
),
28+
ReadmeRuleTestCase(
29+
description="a designated column on the right of <> in upper case",
30+
source="SELECT id FROM customers WHERE 'Carol@example.com' <> BILLING_EMAIL",
31+
expected_texts=("BILLING_EMAIL",),
32+
),
33+
ReadmeRuleTestCase(
34+
description="UPPER does not normalize the other side to lower case",
35+
source="SELECT id FROM customers WHERE LOWER(email) = UPPER('x')",
36+
expected_texts=("email",),
37+
),
38+
ReadmeRuleTestCase(
39+
description="a bare column against a lower-case literal is still not normalized",
40+
source="SELECT id FROM customers WHERE email = 'carol@example.com'",
41+
expected_texts=("email",),
42+
),
43+
ReadmeRuleTestCase(
44+
description="a comparison inside a CTE after a multibyte literal",
45+
source="WITH u AS (SELECT 'é' AS m, email FROM users WHERE email = 'X@y.com') "
46+
"SELECT * FROM u",
47+
expected_texts=("email",),
48+
),
49+
ReadmeRuleTestCase(
50+
description="LOWER on both sides passes",
51+
source="SELECT * FROM u JOIN a ON LOWER(u.email) = LOWER(a.email)",
52+
expected_texts=(),
53+
),
54+
ReadmeRuleTestCase(
55+
description="LOWER against a lower-case literal passes",
56+
source="SELECT id FROM customers WHERE LOWER(email) = 'bob@example.com'",
57+
expected_texts=(),
58+
),
59+
ReadmeRuleTestCase(
60+
description="comments, strings, other columns, LIKE and projections never match",
61+
source="-- email = 'A@b.c'\nSELECT email, 'email = x' AS note FROM customers "
62+
"WHERE email_verified = TRUE AND email LIKE '%dave%'",
63+
expected_texts=(),
64+
),
65+
],
66+
ids=lambda case: case.description,
67+
)
68+
def test_given_readme_email_rule_when_evaluating_then_unnormalized_comparisons_are_reported(
69+
tmp_path: Path, test_case: ReadmeRuleTestCase
70+
) -> None:
71+
rule: RuleCheck = readme_rule(tmp_path)
72+
73+
result: RuleResult = evaluate_rule(
74+
rule=rule,
75+
test_case=RuleCase(
76+
description=test_case.description,
77+
source=test_case.source,
78+
expected_finding_count=len(test_case.expected_texts),
79+
dialect="duckdb",
80+
),
81+
)
82+
texts: tuple[str, ...] = tuple(
83+
test_case.source[finding.start : finding.end] for finding in result.findings
84+
)
85+
86+
assert texts == test_case.expected_texts
87+
assert result.fixed_source == test_case.source

‎uv.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎website/recordings/README.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ The README uses `check-fix`, copied into `.github/demos/`.
1616
- DejaVu Sans Mono, the font the tapes use.
1717
- `sqlclean` on `PATH` with its Python package in the same environment, or `SQLCLEAN_BIN` pointing at
1818
a directory that contains the `sqlclean` to record.
19+
- The `ast` extra (`pip install "sqlclean[ast]"`) for the `check-fix` custom Rule, or
20+
`SQLCLEAN_PYTHON` pointing at an interpreter that has it.
1921

2022
## Record
2123

0 commit comments

Comments
 (0)