feat(rules): add W012 group-by-ordinal#14
feat(rules): add W012 group-by-ordinal#14Pawansingh3889 merged 2 commits intoPawansingh3889:mainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for your first PR on sql-sop! Quick checks:
pytest -qandruff check .pass locally- Every new rule has both a "fires on bad SQL" test AND a
"does NOT fire on safe SQL" test - Rule IDs (
E001,W001, …) are public API — don't renumber
or rename existing ones without the deprecation process in
GOVERNANCE.md CHANGELOG.md[Unreleased]entry for any user-facing change
First-PR-wins on the linked issue applies for 7 days. The
maintainer will review within 14 days of your last response.
|
@mvanhorn — really nice rule. The 1st_quarter test especially: that's the kind of edge case I'd have happily shipped a regression for, so thanks for thinking about it. Heads up on something that's actually my fault: your branch carries the test_duration_tracked >= 0 fix and a CHANGELOG note about it, because PR #13 hasn't landed yet. I'll merge #13 this week, and once main moves, mind rebasing off it? This should drop those two commits cleanly: git fetch upstream Couple of nits, take or leave: The description calls ordinals "non-portable" but Postgres/MySQL/BigQuery/Snowflake all accept them. The reorder-fragility framing in the message feels truer to the actual harm. |
|
Heads up: main has moved on a bit since you opened this. PR #13 (the One small ask while you're rebasing: add an [Unreleased] entry under Added in CHANGELOG.md for W012. Heads up that #15 added a ### Changedblock right above ### Fixed in [Unreleased], so you may need to resolve Two optional thoughts you can take or leave:
Ping me when the rebase is up. |
New warning rule that flags GROUP BY by positional column ordinal: -- should trigger SELECT region, status, COUNT(*) FROM orders GROUP BY 1, 2; -- should pass SELECT region, status, COUNT(*) FROM orders GROUP BY region, status; Why it is a warning, not an error: it parses in every dialect and the result set is correct -- until someone reorders the SELECT list, at which point the grouping silently changes. Flagging it as warning matches how other 'brittle but not broken' patterns are handled. Regex: GROUP\s+BY\s+\d+\b(\s*,\s*\d+\b)* The trailing word boundary on \d+\b means identifiers like 1st_quarter do not match -- '1' is followed by a word character, so the boundary does not hold and no false positive fires. A dedicated test locks that in.
Add [Unreleased] ### Added entry for W012 per maintainer request. Reframe the rule description from "non-portable" to reorder-fragility. Postgres/MySQL/BigQuery/Snowflake all accept ordinals, so portability is not the real harm. The harm is that the grouping silently changes when the SELECT list is reordered.
mvanhorn
force-pushed
the
feat/w012-group-by-ordinal
branch
from
9e20cd7 to
02235b1
Compare
|
Rebased onto main in 02235b1. The PR #13 duration commits dropped out cleanly since my only new commit was the W012 rule. Added the Reframed the rule description: dropped "non-portable" and led with the reorder-fragility framing. You're right that Postgres/MySQL/BigQuery/Snowflake all accept ordinals, so portability isn't the real harm; the brittleness is. On the Verified locally: |
Pawansingh3889
left a comment
Pawansingh3889
left a comment
There was a problem hiding this comment.
Looks good. The reframing away from portability onto reorder-fragility reads cleanly in the CHANGELOG and the commit message, and the 1st_quarter test locks down the word-boundary reasoning. The mixed-list gap you flagged (GROUP BY region, 1) is real but a smaller second-pass problem, fine as a follow-up. Merging as squash.
|
Appreciate W012 landing in the rule set. |
2 participants
Summary
Implements W012 (`group-by-ordinal`) per #2. Warns on `GROUP BY` that groups by positional column ordinal -- the grouping silently breaks the moment someone reorders the SELECT list.
Should fail
```sql
SELECT region, status, COUNT(*)
FROM orders
GROUP BY 1, 2;
```
Should pass
```sql
SELECT region, status, COUNT(*)
FROM orders
GROUP BY region, status;
```
Implementation notes
Test plan
Fixes #2