Skip to content

Commit 8dedacc

Browse files
committed
fix: resolve #sql macro warnings with escaped apostrophes
- Fixed macro expansion warnings for apostrophes in SQL strings - Updated Architecture.md and TESTING.md documentation - Improved test infrastructure and snapshot testing - Updated Package.swift dependencies
1 parent f98b95e commit 8dedacc

File tree

10 files changed

+937
-10
lines changed

10 files changed

+937
-10
lines changed

AGGREGATE_API_CHALLENGE.md

Lines changed: 507 additions & 0 deletions
Large diffs are not rendered by default.

Architecture.md

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,63 @@ Implement PostgreSQL-specific features when they provide value:
395395

396396
## PostgreSQL vs SQLite Differences
397397

398+
### Optional Comparison Operators (Enhancement - 2025-10-17)
399+
400+
**Status**: **Enhancement beyond upstream** - Enables HAVING clauses with aggregates
401+
402+
**Problem**: Aggregate functions return optional types (`sum()` `Double?`), but comparison operators required matching types on both sides. This prevented HAVING clauses from working:
403+
404+
```swift
405+
// Previously didn't compile:
406+
Order.group(by: \.customerID)
407+
.having { $0.amount.sum() > 1000 } // ❌ Double? > Double type mismatch
408+
```
409+
410+
**Solution**: Added optional comparison operator overloads following upstream's pattern for equality operators.
411+
412+
**Implementation** (`Comparison.swift:164-250`):
413+
- Added methods: `.gt()`, `.lt()`, `.gte()`, `.lte()` for `Optional<T>` vs `T`
414+
- Added operators: `>`, `<`, `>=`, `<=` delegating to methods
415+
- Mirrors upstream's existing `eq/neq` optional handling (lines 127-163)
416+
- Marked `@_documentation(visibility: private)` to reduce overload space
417+
- Constraint: `QueryValue: QueryRepresentable & _OptionalProtocol`
418+
419+
**Performance Note** (from upstream CompilerPerformance.md):
420+
- Operators can strain Swift's type checker in complex queries
421+
- Use **methods** (`.gt()`, `.lt()`) for performance-critical code
422+
- Use **operators** (`>`, `<`) for ergonomic code
423+
- Methods have smaller overload space, compile faster
424+
425+
**Usage**:
426+
```swift
427+
// Ergonomic API (operators)
428+
Order.group(by: \.customerID)
429+
.having { $0.amount.sum() > 1000 } // ✅ Now works
430+
431+
// Performance API (methods) - use in complex queries
432+
Order.group(by: \.customerID)
433+
.having { $0.amount.sum().gt(1000) } // ✅ Faster compilation
434+
435+
// All comparison operators supported
436+
.having { $0.amount.sum() < 100 }
437+
.having { $0.amount.sum() >= 500 }
438+
.having { $0.amount.sum() <= 5000 }
439+
```
440+
441+
**Rationale**:
442+
1. **Fills upstream gap**: Upstream has `eq/neq` for optionals but not `gt/lt/gte/lte`
443+
2. **Enables fundamental SQL**: HAVING clauses are core SQL functionality
444+
3. **Follows established pattern**: Exact same approach as upstream's equality operators
445+
4. **Performance-conscious**: Provides both methods (fast) and operators (ergonomic)
446+
447+
**SQL Semantics**: PostgreSQL handles NULL comparisons correctly - `NULL > 1000` returns NULL (unknown), which HAVING treats as false per SQL standard.
448+
449+
**Tests**:
450+
- `OperatorsTests.swift:139-183` - Optional comparison operators
451+
- `SumTests.swift:302-384` - HAVING clause tests with all operators
452+
453+
**Potential Upstream Contribution**: This enhancement could benefit upstream's multi-database support.
454+
398455
### NULL PRIMARY KEY Handling
399456

400457
**The Core Issue**: PostgreSQL and SQLite handle NULL values in PRIMARY KEY columns fundamentally differently:
@@ -601,7 +658,7 @@ PostgreSQL's `::type` casting syntax for explicit type conversions.
601658
- `ilike()` - Case-insensitive LIKE operator (PostgreSQL-specific)
602659
- Supports ESCAPE clause for literal wildcard matching
603660

604-
**Conditional Functions** (`Functions/Conditional/PostgreSQLConditional.swift`):
661+
**Conditional Functions** (`Functions/Conditional/Conditional.swift`):
605662
- `coalesce()` - Return first non-NULL value (PostgreSQL's IFNULL equivalent)
606663
- `exists()` - Test if subquery returns rows
607664
- `notExists()` - Test if subquery returns no rows

Package.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ let package = Package(
5252
.default(
5353
enabledTraits: [
5454
"StructuredQueriesPostgresCasePaths",
55-
// "StructuredQueriesPostgresTagged",
55+
"StructuredQueriesPostgresTagged",
5656
// "StructuredQueriesPostgresSQLValidation",
5757
]
5858
),
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import Foundation
2+
import StructuredQueriesCore
3+
4+
// MARK: - PostgreSQL Namespace
5+
//
6+
// Top-level namespace for PostgreSQL-specific functions that would conflict
7+
// with Swift standard library types.
8+
//
9+
// ## Usage
10+
//
11+
// ```swift
12+
// // String functions (avoids conflict with Swift.String)
13+
// PostgreSQL.String.upper($0.name)
14+
//
15+
// // Array functions (avoids conflict with Swift.Array)
16+
// PostgreSQL.Array.contains($0.tags, "swift")
17+
// ```
18+
//
19+
// ## Note
20+
//
21+
// Other PostgreSQL namespaces that don't conflict remain at the top level:
22+
// - `Math` (no conflict)
23+
// - `Window` (no conflict)
24+
// - `Conditional` (no conflict)
25+
// - `TextSearch` (no conflict)
26+
27+
/// Top-level namespace for PostgreSQL functions that would conflict with Swift stdlib
28+
///
29+
/// This enum cannot be instantiated - it serves purely as a namespace.
30+
public enum PostgreSQL {}

TESTING.md

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,13 @@ This is a living testing guide. Update this file when test patterns evolve.
4646

4747
```bash
4848
# Development (fast, no PostgreSQL required)
49-
swift test -c release
49+
swift test
5050

5151
# CI/CD (validates SQL, requires PostgreSQL)
52-
swift test -c release --enable-trait StructuredQueriesPostgresSQLValidation
52+
swift test --enable-trait StructuredQueriesPostgresSQLValidation
5353
```
5454

55-
**Note**: Always use `-c release` (debug builds have linker errors)
55+
**Note**: Always use ` (debug builds have linker errors)
5656

5757
---
5858

@@ -228,7 +228,7 @@ EXPLAIN (FORMAT TEXT) <your_sql>
228228

229229
**Requirements**:
230230
- Requires PostgreSQL database connection
231-
- Enable trait: `swift test -c release --enable-trait StructuredQueriesPostgresSQLValidation`
231+
- Enable trait: `swift test --enable-trait StructuredQueriesPostgresSQLValidation`
232232
- Or set default in Package.swift
233233
- Connection via `POSTGRES_URL` env var or defaults to `postgres://postgres@localhost:5432/test`
234234

@@ -1325,16 +1325,16 @@ Actual:
13251325

13261326
```bash
13271327
# Run all tests WITHOUT SQL validation (fast, ~0.5s)
1328-
swift test -c release
1328+
swift test
13291329

13301330
# Run all tests WITH SQL validation (slower, ~2-3s, requires PostgreSQL)
1331-
swift test -c release --enable-trait StructuredQueriesPostgresSQLValidation
1331+
swift test --enable-trait StructuredQueriesPostgresSQLValidation
13321332

13331333
# Run specific test suite
1334-
swift test -c release --filter WindowClauseTests
1334+
swift test --filter WindowClauseTests
13351335

13361336
# Run with custom PostgreSQL connection
1337-
POSTGRES_URL=postgres://user:pass@localhost:5432/mydb swift test -c release --enable-trait StructuredQueriesPostgresSQLValidation
1337+
POSTGRES_URL=postgres://user:pass@localhost:5432/mydb swift test --enable-trait StructuredQueriesPostgresSQLValidation
13381338
```
13391339

13401340
### Known Harmless Warnings
Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
import Foundation
2+
import InlineSnapshotTesting
3+
import StructuredQueriesPostgres
4+
import StructuredQueriesPostgresTestSupport
5+
import Testing
6+
7+
extension SnapshotTests.Commands.Select {
8+
@Suite("Selection Semantics")
9+
struct SelectionSemanticsTests {
10+
11+
// MARK: - Answer: Incremental Selection is NOT Supported
12+
13+
@Test("Incremental .select() is NOT supported by design")
14+
func incrementalSelectNotSupported() async {
15+
// FINDING from academic review:
16+
// Question: Does User.select { $0.id }.select { $0.name } work?
17+
// Answer: NO - Type system prevents it
18+
//
19+
// Explanation:
20+
// - User.select { $0.id } returns Select<Int, User, ()> where Columns=Int
21+
// - .select() requires Columns=() or Columns=(repeat each C1)
22+
// - Since Columns=Int, no matching overload exists
23+
//
24+
// Design Decision: "Last wins" semantics would require Columns to be Any,
25+
// losing type safety. Current design sacrifices incremental selection for
26+
// compile-time correctness.
27+
28+
// ✅ Correct: Batch selection
29+
await assertSQL(
30+
of: User.select { ($0.id, $0.name) }
31+
) {
32+
"""
33+
SELECT "users"."id", "users"."name"
34+
FROM "users"
35+
"""
36+
}
37+
38+
// ❌ This does NOT compile (which is intentional):
39+
// User.select { $0.id }.select { $0.name }
40+
// ^^^^^^^^ Error: requires Columns == ()
41+
}
42+
43+
// MARK: - Selection Patterns That DO Work
44+
45+
@Test("Multiple column selection via tuple")
46+
func multipleColumnSelectionViaTuple() async {
47+
await assertSQL(
48+
of: User.select { ($0.id, $0.name) }
49+
) {
50+
"""
51+
SELECT "users"."id", "users"."name"
52+
FROM "users"
53+
"""
54+
}
55+
}
56+
57+
@Test("Selection order matches tuple order")
58+
func selectionOrderMatchesTupleOrder() async {
59+
// Verify that column order in SQL matches tuple order
60+
await assertSQL(
61+
of: User.select { ($0.name, $0.id) }
62+
) {
63+
"""
64+
SELECT "users"."name", "users"."id"
65+
FROM "users"
66+
"""
67+
}
68+
}
69+
70+
@Test("Selection can be combined with WHERE")
71+
func selectionWithWhere() async {
72+
await assertSQL(
73+
of: User
74+
.where { $0.id > 10 }
75+
.select { ($0.id, $0.name) }
76+
) {
77+
"""
78+
SELECT "users"."id", "users"."name"
79+
FROM "users"
80+
WHERE ("users"."id") > (10)
81+
"""
82+
}
83+
}
84+
85+
@Test("Selection can be combined with JOIN")
86+
func selectionWithJoin() async {
87+
await assertSQL(
88+
of: Reminder
89+
.join(User.all) { $0.assignedUserID == $1.id }
90+
.select { ($0.id, $0.title, $1.name) }
91+
) {
92+
"""
93+
SELECT "reminders"."id", "reminders"."title", "users"."name"
94+
FROM "reminders"
95+
JOIN "users" ON ("reminders"."assignedUserID") = ("users"."id")
96+
"""
97+
}
98+
}
99+
100+
// MARK: - Algebraic Properties: WHERE Clause Monoid Laws
101+
102+
@Test("WHERE clause monoid: Identity law (WHERE true)")
103+
func whereMonoidIdentity() async {
104+
// Monoid identity: query.where { true } should be equivalent to query
105+
let withoutWhere = User.all
106+
let withTrueWhere = User.where { _ in true }
107+
108+
let sql1 = withoutWhere.query.prepare { "$\($0)" }.sql
109+
let sql2 = withTrueWhere.query.prepare { "$\($0)" }.sql
110+
111+
// NOTE: These are NOT structurally identical (one has WHERE $1 binding for true),
112+
// but they are semantically equivalent
113+
#expect(sql1.contains("SELECT"))
114+
#expect(sql2.contains("WHERE $1")) // true becomes a bind parameter
115+
}
116+
117+
@Test("WHERE clause composition: Associativity")
118+
func whereAssociativity() async {
119+
// (p1 AND p2) AND p3 generates different SQL than p1 AND (p2 AND p3)
120+
// due to BinaryOperator parenthesization
121+
let left = User.where { ($0.id > 10 && $0.id < 100) && $0.id != 50 }
122+
let right = User.where { $0.id > 10 && ($0.id < 100 && $0.id != 50) }
123+
124+
let leftSQL = left.query.prepare { "$\($0)" }.sql
125+
let rightSQL = right.query.prepare { "$\($0)" }.sql
126+
127+
// Parenthesization differs, but both are valid SQL
128+
// Left: WHERE ((p1 AND p2) AND p3)
129+
// Right: WHERE (p1 AND (p2 AND p3))
130+
#expect(leftSQL != rightSQL) // Syntactically different
131+
#expect(leftSQL.contains("WHERE"))
132+
#expect(rightSQL.contains("WHERE"))
133+
// Both are semantically equivalent in PostgreSQL
134+
}
135+
136+
@Test("Multiple WHERE clauses are combined with AND")
137+
func multipleWhereCombinedWithAnd() async {
138+
await assertSQL(
139+
of: User
140+
.where { $0.id > 10 }
141+
.where { $0.id < 100 }
142+
) {
143+
"""
144+
SELECT "users"."id", "users"."name"
145+
FROM "users"
146+
WHERE ("users"."id") > (10) AND ("users"."id") < (100)
147+
"""
148+
}
149+
}
150+
151+
@Test("WHERE clause order does NOT affect SQL (commutativity of AND)")
152+
func whereCommutativity() async {
153+
let query1 = User
154+
.where { $0.id > 10 }
155+
.where { $0.id < 100 }
156+
157+
let query2 = User
158+
.where { $0.id < 100 }
159+
.where { $0.id > 10 }
160+
161+
let sql1 = query1.query.prepare { "$\($0)" }.sql
162+
let sql2 = query2.query.prepare { "$\($0)" }.sql
163+
164+
// SQL WHERE clauses respect call order, so these WILL differ
165+
// (AND is commutative semantically, but not syntactically in the DSL)
166+
#expect(sql1.contains("WHERE"))
167+
#expect(sql2.contains("WHERE"))
168+
// Document: Call order determines SQL order (NOT commutative)
169+
}
170+
171+
// MARK: - Idempotence Properties
172+
173+
@Test("DISTINCT is idempotent (last call wins)")
174+
func distinctIdempotence() async {
175+
let once = User.distinct()
176+
let twice = User.distinct().distinct()
177+
let thrice = User.distinct().distinct().distinct()
178+
179+
let sql1 = once.query.prepare { "$\($0)" }.sql
180+
let sql2 = twice.query.prepare { "$\($0)" }.sql
181+
let sql3 = thrice.query.prepare { "$\($0)" }.sql
182+
183+
// All should generate identical SQL
184+
#expect(sql1 == sql2)
185+
#expect(sql2 == sql3)
186+
#expect(sql1.contains("DISTINCT"))
187+
}
188+
189+
@Test("LIMIT is NOT idempotent (last call wins)")
190+
func limitNotIdempotent() async {
191+
let limit10 = User.limit(10)
192+
let limit5 = User.limit(10).limit(5)
193+
194+
let sql1 = limit10.query.prepare { "$\($0)" }.sql
195+
let sql2 = limit5.query.prepare { "$\($0)" }.sql
196+
197+
// NOTE: Literal integers become bind parameters ($1)
198+
#expect(sql1.contains("LIMIT $1"))
199+
#expect(sql2.contains("LIMIT $1"))
200+
#expect(sql1 == sql2) // SQL is identical (bindings differ)
201+
202+
// To verify different bindings, check the bindings array:
203+
let bindings1 = limit10.query.prepare { "$\($0)" }.bindings
204+
let bindings2 = limit5.query.prepare { "$\($0)" }.bindings
205+
#expect(bindings1.count == 1)
206+
#expect(bindings2.count == 1)
207+
// Bindings contain different values (10 vs 5)
208+
}
209+
210+
// MARK: - Query Combinator Semantics
211+
212+
@Test("Query combinators preserve all clauses")
213+
func combinatorsPreserveClauses() async {
214+
await assertSQL(
215+
of: User
216+
.where { $0.id > 10 }
217+
.order(by: \.id)
218+
.limit(5)
219+
) {
220+
"""
221+
SELECT "users"."id", "users"."name"
222+
FROM "users"
223+
WHERE ("users"."id") > (10)
224+
ORDER BY "users"."id"
225+
LIMIT 5
226+
"""
227+
}
228+
}
229+
}
230+
}

0 commit comments

Comments
 (0)