Skip to content

Commit 85f51e9

Browse files
committed
address gpt5 review
1 parent 42f81bb commit 85f51e9

File tree

9 files changed

+183
-35
lines changed

9 files changed

+183
-35
lines changed

packages/db/src/errors.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,11 +350,18 @@ export class LimitOffsetRequireOrderByError extends QueryCompilationError {
350350
}
351351

352352
export class CollectionInputNotFoundError extends QueryCompilationError {
353-
constructor(alias: string, collectionId?: string) {
353+
constructor(
354+
alias: string,
355+
collectionId?: string,
356+
availableKeys?: Array<string>
357+
) {
354358
const details = collectionId
355359
? `alias "${alias}" (collection "${collectionId}")`
356360
: `collection "${alias}"`
357-
super(`Input for ${details} not found in inputs map`)
361+
const availableKeysMsg = availableKeys?.length
362+
? `. Available keys: ${availableKeys.join(`, `)}`
363+
: ``
364+
super(`Input for ${details} not found in inputs map${availableKeysMsg}`)
358365
}
359366
}
360367

packages/db/src/query/builder/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ export type SchemaFromSource<T extends Source> = Prettify<{
104104
* GetAliases - Extracts all table aliases available in a query context
105105
*
106106
* Simple utility type that returns the keys of the schema, representing
107-
* all table/collection aliases that can be referenced in the current query.
107+
* all table/source aliases that can be referenced in the current query.
108108
*/
109109
export type GetAliases<TContext extends Context> = keyof TContext[`schema`]
110110

packages/db/src/query/compiler/index.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ export interface CompilationResult {
3838
collectionId: string
3939
/** The compiled query pipeline */
4040
pipeline: ResultStream
41-
/** Map of collection aliases to their WHERE clauses for index optimization */
42-
collectionWhereClauses: Map<string, BasicExpression<boolean>>
41+
/** Map of source aliases to their WHERE clauses for index optimization */
42+
sourceWhereClauses: Map<string, BasicExpression<boolean>>
4343
/** Map of alias to underlying collection id used during compilation */
4444
aliasToCollectionId: Record<string, string>
4545
}
@@ -70,8 +70,7 @@ export function compileQuery(
7070
}
7171

7272
// Optimize the query before compilation
73-
const { optimizedQuery: query, collectionWhereClauses } =
74-
optimizeQuery(rawQuery)
73+
const { optimizedQuery: query, sourceWhereClauses } = optimizeQuery(rawQuery)
7574

7675
// Create mapping from optimized query to original for caching
7776
queryMapping.set(query, rawQuery)
@@ -81,10 +80,11 @@ export function compileQuery(
8180
const allInputs = { ...inputs }
8281

8382
// Track alias to collection id relationships discovered during compilation so
84-
// the live layer can subscribe to every alias the optimiser introduces.
83+
// the live layer can subscribe to every alias the optimizer introduces.
8584
const aliasToCollectionId: Record<string, string> = {}
8685

8786
// Create a map of table aliases to inputs
87+
// Note: alias keys take precedence over collection keys for input resolution
8888
const tables: Record<string, KeyedStream> = {}
8989

9090
// Process the FROM clause to get the main table
@@ -294,7 +294,7 @@ export function compileQuery(
294294
const compilationResult = {
295295
collectionId: mainCollectionId,
296296
pipeline: result,
297-
collectionWhereClauses,
297+
sourceWhereClauses,
298298
aliasToCollectionId,
299299
}
300300
cache.set(rawQuery, compilationResult)
@@ -323,7 +323,7 @@ export function compileQuery(
323323
const compilationResult = {
324324
collectionId: mainCollectionId,
325325
pipeline: result,
326-
collectionWhereClauses,
326+
sourceWhereClauses,
327327
aliasToCollectionId,
328328
}
329329
cache.set(rawQuery, compilationResult)
@@ -350,7 +350,11 @@ function processFrom(
350350
case `collectionRef`: {
351351
const input = allInputs[from.alias] ?? allInputs[from.collection.id]
352352
if (!input) {
353-
throw new CollectionInputNotFoundError(from.alias, from.collection.id)
353+
throw new CollectionInputNotFoundError(
354+
from.alias,
355+
from.collection.id,
356+
Object.keys(allInputs)
357+
)
354358
}
355359
aliasToCollectionId[from.alias] = from.collection.id
356360
return { alias: from.alias, input, collectionId: from.collection.id }

packages/db/src/query/compiler/joins.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,9 @@ function processJoin(
264264
[key: unknown, [originalKey: string, namespacedRow: NamespacedRow]]
265265
> = activePipeline.pipe(
266266
tap((data) => {
267+
// For outer joins (LEFT/RIGHT), the driving side determines which alias's
268+
// subscription we consult for lazy loading. The main table drives LEFT joins,
269+
// joined table drives RIGHT joins.
267270
const lazyAliasCandidate =
268271
activeCollection === `main` ? joinedTableAlias : mainTableAlias
269272
const lazyCollectionSubscription =
@@ -419,7 +422,11 @@ function processJoinSource(
419422
case `collectionRef`: {
420423
const input = allInputs[from.alias] ?? allInputs[from.collection.id]
421424
if (!input) {
422-
throw new CollectionInputNotFoundError(from.alias, from.collection.id)
425+
throw new CollectionInputNotFoundError(
426+
from.alias,
427+
from.collection.id,
428+
Object.keys(allInputs)
429+
)
423430
}
424431
aliasToCollectionId[from.alias] = from.collection.id
425432
return { alias: from.alias, input, collectionId: from.collection.id }

packages/db/src/query/live/collection-config-builder.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,11 @@ export class CollectionConfigBuilder<
5050
private graphCache: D2 | undefined
5151
private inputsCache: Record<string, RootStreamBuilder<unknown>> | undefined
5252
private pipelineCache: ResultStream | undefined
53-
public collectionWhereClausesCache:
53+
public sourceWhereClausesCache:
5454
| Map<string, BasicExpression<boolean>>
5555
| undefined
5656

57-
// Map of collection alias to subscription
57+
// Map of source alias to subscription
5858
readonly subscriptions: Record<string, CollectionSubscription> = {}
5959
// Map of collection IDs to functions that load keys for that lazy collection
6060
lazyCollectionsCallbacks: Record<string, LazyCollectionCallbacks> = {}
@@ -118,7 +118,7 @@ export class CollectionConfigBuilder<
118118
if (collection) {
119119
return collection.id
120120
}
121-
throw new Error(`Unknown collection alias "${alias}"`)
121+
throw new Error(`Unknown source alias "${alias}"`)
122122
}
123123

124124
// The callback function is called after the graph has run.
@@ -209,12 +209,19 @@ export class CollectionConfigBuilder<
209209
this.graphCache = undefined
210210
this.inputsCache = undefined
211211
this.pipelineCache = undefined
212-
this.collectionWhereClausesCache = undefined
212+
this.sourceWhereClausesCache = undefined
213213

214214
// Reset lazy collection state
215215
this.lazyCollections.clear()
216216
this.optimizableOrderByCollections = {}
217217
this.lazyCollectionsCallbacks = {}
218+
219+
// Clear subscription references to prevent memory leaks
220+
// Note: Individual subscriptions are already unsubscribed via unsubscribeCallbacks
221+
Object.keys(this.subscriptions).forEach(
222+
(key) => delete this.subscriptions[key]
223+
)
224+
this.compiledAliasToCollectionId = {}
218225
}
219226
}
220227

@@ -239,7 +246,7 @@ export class CollectionConfigBuilder<
239246
)
240247

241248
this.pipelineCache = compilation.pipeline
242-
this.collectionWhereClausesCache = compilation.collectionWhereClauses
249+
this.sourceWhereClausesCache = compilation.sourceWhereClauses
243250
this.compiledAliasToCollectionId = compilation.aliasToCollectionId
244251
// Optimized queries can introduce aliases beyond those declared on the
245252
// builder. If that happens, provision inputs for the missing aliases and
@@ -266,7 +273,7 @@ export class CollectionConfigBuilder<
266273
)
267274

268275
this.pipelineCache = compilation.pipeline
269-
this.collectionWhereClausesCache = compilation.collectionWhereClauses
276+
this.sourceWhereClausesCache = compilation.sourceWhereClauses
270277
this.compiledAliasToCollectionId = compilation.aliasToCollectionId
271278
}
272279
}

packages/db/src/query/live/collection-subscriber.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -226,12 +226,12 @@ export class CollectionSubscriber<
226226
}
227227

228228
private getWhereClauseForAlias(): BasicExpression<boolean> | undefined {
229-
const collectionWhereClausesCache =
230-
this.collectionConfigBuilder.collectionWhereClausesCache
231-
if (!collectionWhereClausesCache) {
229+
const sourceWhereClausesCache =
230+
this.collectionConfigBuilder.sourceWhereClausesCache
231+
if (!sourceWhereClausesCache) {
232232
return undefined
233233
}
234-
return collectionWhereClausesCache.get(this.alias)
234+
return sourceWhereClausesCache.get(this.alias)
235235
}
236236

237237
private getOrderByInfo(): OrderByOptimizationInfo | undefined {

packages/db/src/query/optimizer.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,8 @@ export interface GroupedWhereClauses {
162162
export interface OptimizationResult {
163163
/** The optimized query with WHERE clauses potentially moved to subqueries */
164164
optimizedQuery: QueryIR
165-
/** Map of collection aliases to their extracted WHERE clauses for index optimization */
166-
collectionWhereClauses: Map<string, BasicExpression<boolean>>
165+
/** Map of source aliases to their extracted WHERE clauses for index optimization */
166+
sourceWhereClauses: Map<string, BasicExpression<boolean>>
167167
}
168168

169169
/**
@@ -184,14 +184,14 @@ export interface OptimizationResult {
184184
* where: [eq(u.dept_id, 1), gt(p.views, 100)]
185185
* }
186186
*
187-
* const { optimizedQuery, collectionWhereClauses } = optimizeQuery(originalQuery)
187+
* const { optimizedQuery, sourceWhereClauses } = optimizeQuery(originalQuery)
188188
* // Result: Single-source clauses moved to deepest possible subqueries
189-
* // collectionWhereClauses: Map { 'u' => eq(u.dept_id, 1), 'p' => gt(p.views, 100) }
189+
* // sourceWhereClauses: Map { 'u' => eq(u.dept_id, 1), 'p' => gt(p.views, 100) }
190190
* ```
191191
*/
192192
export function optimizeQuery(query: QueryIR): OptimizationResult {
193-
// First, extract collection WHERE clauses before optimization
194-
const collectionWhereClauses = extractCollectionWhereClauses(query)
193+
// First, extract source WHERE clauses before optimization
194+
const sourceWhereClauses = extractSourceWhereClauses(query)
195195

196196
// Apply multi-level predicate pushdown with iterative convergence
197197
let optimized = query
@@ -214,7 +214,7 @@ export function optimizeQuery(query: QueryIR): OptimizationResult {
214214

215215
return {
216216
optimizedQuery: cleaned,
217-
collectionWhereClauses,
217+
sourceWhereClauses,
218218
}
219219
}
220220

@@ -224,16 +224,16 @@ export function optimizeQuery(query: QueryIR): OptimizationResult {
224224
* to specific collections, but only for simple queries without joins.
225225
*
226226
* @param query - The original QueryIR to analyze
227-
* @returns Map of collection aliases to their WHERE clauses
227+
* @returns Map of source aliases to their WHERE clauses
228228
*/
229-
function extractCollectionWhereClauses(
229+
function extractSourceWhereClauses(
230230
query: QueryIR
231231
): Map<string, BasicExpression<boolean>> {
232-
const collectionWhereClauses = new Map<string, BasicExpression<boolean>>()
232+
const sourceWhereClauses = new Map<string, BasicExpression<boolean>>()
233233

234234
// Only analyze queries that have WHERE clauses
235235
if (!query.where || query.where.length === 0) {
236-
return collectionWhereClauses
236+
return sourceWhereClauses
237237
}
238238

239239
// Split all AND clauses at the root level for granular analysis
@@ -254,12 +254,12 @@ function extractCollectionWhereClauses(
254254
if (isCollectionReference(query, sourceAlias)) {
255255
// Check if the WHERE clause can be converted to collection-compatible format
256256
if (isConvertibleToCollectionFilter(whereClause)) {
257-
collectionWhereClauses.set(sourceAlias, whereClause)
257+
sourceWhereClauses.set(sourceAlias, whereClause)
258258
}
259259
}
260260
}
261261

262-
return collectionWhereClauses
262+
return sourceWhereClauses
263263
}
264264

265265
/**

packages/db/tests/query/compiler/subqueries.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,77 @@ describe(`Query2 Subqueries`, () => {
352352
})
353353

354354
describe(`Complex composable queries`, () => {
355+
it(`exports alias metadata from nested subqueries`, () => {
356+
// Create a nested subquery structure to test alias metadata propagation
357+
const innerQuery = new Query()
358+
.from({ user: usersCollection })
359+
.where(({ user }) => eq(user.status, `active`))
360+
361+
const middleQuery = new Query()
362+
.from({ activeUser: innerQuery })
363+
.select(({ activeUser }) => ({
364+
id: activeUser.id,
365+
name: activeUser.name,
366+
}))
367+
368+
const outerQuery = new Query()
369+
.from({ issue: issuesCollection })
370+
.join({ userInfo: middleQuery }, ({ issue, userInfo }) =>
371+
eq(issue.userId, userInfo.id)
372+
)
373+
.select(({ issue, userInfo }) => ({
374+
issueId: issue.id,
375+
issueTitle: issue.title,
376+
userName: userInfo?.name,
377+
}))
378+
379+
const builtQuery = getQueryIR(outerQuery)
380+
381+
const usersSubscription = usersCollection.subscribeChanges(() => {})
382+
const issuesSubscription = issuesCollection.subscribeChanges(() => {})
383+
const subscriptions: Record<string, CollectionSubscription> = {
384+
[usersCollection.id]: usersSubscription,
385+
[issuesCollection.id]: issuesSubscription,
386+
}
387+
388+
const dummyCallbacks = {
389+
loadKeys: (_: any) => {},
390+
loadInitialState: () => {},
391+
}
392+
393+
// Compile the query
394+
const graph = new D2()
395+
const issuesInput = createIssueInput(graph)
396+
const usersInput = createUserInput(graph)
397+
const lazyCollections = new Set<string>()
398+
const compilation = compileQuery(
399+
builtQuery,
400+
{
401+
issues: issuesInput,
402+
users: usersInput,
403+
},
404+
{ issues: issuesCollection, users: usersCollection },
405+
subscriptions,
406+
{ issues: dummyCallbacks, users: dummyCallbacks },
407+
lazyCollections,
408+
{}
409+
)
410+
411+
// Verify that alias metadata includes aliases from the query
412+
const aliasToCollectionId = compilation.aliasToCollectionId
413+
414+
// Should include the main table alias (note: alias is 'issue', not 'issues')
415+
expect(aliasToCollectionId.issue).toBe(issuesCollection.id)
416+
417+
// Should include the user alias from the subquery
418+
expect(aliasToCollectionId.user).toBe(usersCollection.id)
419+
420+
// Verify that the compiler correctly maps aliases to collection IDs
421+
expect(Object.keys(aliasToCollectionId)).toHaveLength(2)
422+
expect(aliasToCollectionId.issue).toBe(issuesCollection.id)
423+
expect(aliasToCollectionId.user).toBe(usersCollection.id)
424+
})
425+
355426
it(`executes simple aggregate subquery`, () => {
356427
// Create a base query that filters issues for project 1
357428
const baseQuery = new Query()

0 commit comments

Comments
 (0)