mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-11 21:51:48 +00:00
ci(postgres): teach the parity guide the DISTINCT row-count trap + refactor faithfulness
Two review lessons from the post-merge net-diff/whole-repo re-audit of the SQL-dialect classes: - Section 3 (row-count trap) now covers SELECT DISTINCT too: adding the ORDER BY column to the select to satisfy Postgres grows the DISTINCT key and changes the MariaDB row count when the column is not single-valued per distinct row -- sort in Python instead. - New section 6: a 'refactor' / raw-SQL->qb conversion is not automatically 1:1. Diff the WHERE/predicate and the resulting row set, not just the SELECT shape -- a conversion that widens a filter (e.g. posting_datetime > X gaining an OR (== X AND creation > ...) branch under a sql->qb refactor) changes the rows touched on both engines and hides under a refactor label. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
35
.github/POSTGRES_COMPATIBILITY.md
vendored
35
.github/POSTGRES_COMPATIBILITY.md
vendored
@@ -45,7 +45,9 @@ Flag a changed query that uses any of these:
|
||||
- **`HAVING` referencing a `SELECT` alias** — PostgreSQL rejects output-column aliases in
|
||||
`HAVING` (regardless of whether the query has a `GROUP BY`; MariaDB allows them). Repeat the
|
||||
underlying expression in `HAVING`, or move a non-aggregate predicate into `WHERE`.
|
||||
- **`SELECT DISTINCT … ORDER BY <expr not in the select list>`** — add the expr to the select.
|
||||
- **`SELECT DISTINCT … ORDER BY <expr not in the select list>`** — add the expr to the select
|
||||
**only if it is single-valued per distinct row**; otherwise it grows the `DISTINCT` key and the
|
||||
MariaDB row count (see §3) — drop the SQL `ORDER BY` and sort in Python instead.
|
||||
- **Single-quoted column alias** `AS 'x'` — PostgreSQL reads `'x'` as a string literal. Use an
|
||||
unquoted (or double-quoted) alias.
|
||||
- **`varchar | varchar`** (bitwise OR misused as a coalesce) — errors on PostgreSQL. Use
|
||||
@@ -119,7 +121,7 @@ These don't error, so a one-engine CI stays green. Flag them:
|
||||
|
||||
---
|
||||
|
||||
## 3. The `GROUP BY` row-count trap (the single most important rule)
|
||||
## 3. The row-count trap — `GROUP BY` **and** `DISTINCT` (the single most important rule)
|
||||
|
||||
When making a loose `GROUP BY` PostgreSQL-valid, **do not add a non-functionally-dependent
|
||||
column to the `GROUP BY` just to satisfy PostgreSQL** — that turns one group row into N and
|
||||
@@ -140,6 +142,14 @@ versa) to make a number "more correct" — that changes the MariaDB value. The w
|
||||
MariaDB's prior one-value-per-group output; a different aggregate is a product change, out of
|
||||
scope for a portability fix.
|
||||
|
||||
**The same trap applies to `SELECT DISTINCT`.** To satisfy PostgreSQL's "an `ORDER BY` expr must
|
||||
appear in the select list under `DISTINCT`" rule, **do not blindly add the ordered column to the
|
||||
select** — if it is not single-valued per existing distinct row, the `DISTINCT` key grows and
|
||||
MariaDB returns **more rows** (a regression), exactly as adding a non-FD column to `GROUP BY` does.
|
||||
Add it only when it is functionally dependent on the existing select columns; otherwise drop the
|
||||
SQL `ORDER BY` and **sort in Python** (`key=str.casefold`, per §2) so the distinct row set is
|
||||
unchanged.
|
||||
|
||||
---
|
||||
|
||||
## 4. False positives — do NOT flag these
|
||||
@@ -170,10 +180,27 @@ These are auto-handled by the framework and are **not** breaks:
|
||||
|
||||
---
|
||||
|
||||
## 6. Refactors and raw-SQL→ORM conversions are not automatically 1:1
|
||||
|
||||
A commit labeled a **refactor** or a **raw-`frappe.db.sql` → `frappe.qb`/ORM conversion** is meant
|
||||
to preserve behaviour — but it easily doesn't, and the change passes the static checker and a
|
||||
one-engine green run. **Diff the `WHERE`/predicate, the `JOIN`/`ON` conditions, and the resulting
|
||||
row set — not just the `SELECT` shape.** A conversion that silently widens or narrows the filter
|
||||
changes the rows touched on **both** engines and is a regression hiding under a "refactor" label.
|
||||
|
||||
Real example: an `UPDATE` whose bound was `posting_datetime > X` gained an
|
||||
`OR (posting_datetime == X AND creation > args.creation)` branch during a "`sql` → `qb` refactor",
|
||||
widening the rows updated on both engines. Even when such a change is a deliberate bug-fix it must
|
||||
be called out and tested — it is **not** the no-op the refactor label implies. Confirm the
|
||||
converted query touches exactly the same rows with the same values MariaDB produced before.
|
||||
|
||||
---
|
||||
|
||||
## How to review
|
||||
|
||||
For every changed query: does it (a) use a construct from §1 (would error on PostgreSQL), or
|
||||
(b) match a divergence in §2/§3 (different result across engines)? If so, comment with the
|
||||
For every changed query: does it (a) use a construct from §1 (would error on PostgreSQL),
|
||||
(b) match a divergence in §2/§3 (different result across engines), or (c) change the row set under
|
||||
a refactor/conversion label (§6)? If so, comment with the
|
||||
portable fix and confirm it leaves **MariaDB output unchanged**. Skip the §4 false positives.
|
||||
Prefer a comment that names the rule (e.g. "loose GROUP BY — Max()-wrap, don't add to GROUP BY:
|
||||
splits the row count") so the fix is unambiguous.
|
||||
|
||||
Reference in New Issue
Block a user