fix: scalar subquery in select list should infer correct type #412
Conversation
🦋 Changeset detectedLatest commit: f4f50e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughRestructures AST description to return Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as getASTDescription (parent)
participant NodeDesc as getDescribedNode
participant SelectDesc as getDescribedSelectStmt
participant SubCaller as getASTDescription (subquery)
participant SubLink as getSubLinkType
Caller->>NodeDesc: describe AST node
NodeDesc->>SelectDesc: if node is SelectStmt -> route to getDescribedSelectStmt
SelectDesc->>SelectDesc: build sub-ParseResult (types, cols, relations)
SelectDesc->>SubCaller: call getASTDescription(sub-ParseResult)
SubCaller-->>SelectDesc: return { map, meta } (columns + meta)
SelectDesc->>SelectDesc: pick first column -> produce single-column description
SelectDesc-->>NodeDesc: return described SelectStmt node
NodeDesc-->>Caller: incorporate described node into parent map
Note right of SubLink: getSubLinkType centralizes EXISTS/EXPR_SUBLINK decision and return-type handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/generate/src/ast-describe.ts(3 hunks)packages/generate/src/generate.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/generate/src/ast-describe.ts (2)
packages/ast-types/src/index.ts (2)
SelectStmt(887-908)ParseResult(4-7)packages/generate/src/utils/get-relations-with-joins.ts (2)
flattenRelationsWithJoinsMap(81-93)getRelationsWithJoins(11-27)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/wet-ears-cross.md (1)
5-5: Clarify the changeset description to remove redundancy and improve clarity.The description contains awkward phrasing with "type inference" repeated. Consider a more concise and formal wording.
Apply this diff to improve clarity:
-Fixed an issue with type inference for scalar subquery type inference. +Fixed type inference for scalar subqueries in select lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/wet-ears-cross.md(1 hunks)packages/generate/src/generate.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/generate/src/generate.test.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/wet-ears-cross.md
[style] ~4-~4: Consider using a different verb for a more formal wording.
Context: --- "@ts-safeql/generate": patch --- Fixed an issue with type inference for scalar...
(FIX_RESOLVE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, 20)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/generate/src/ast-describe.ts (1)
1-1: Fix the Prettier formatting issue.The CI pipeline reports a formatting error. Please run
prettier --write packages/generate/src/ast-describe.tsto fix.
🧹 Nitpick comments (1)
packages/generate/src/ast-describe.ts (1)
523-589: LGTM! Subquery type inference properly implemented.The refactor centralizes SubLink type resolution in
getSubLinkTypeand addsgetDescribedSelectStmtto recursively describe scalar subqueries. The syntheticParseResultwithversion: 0is used to bootstrap the recursive description—this should be fine if the version field isn't used for type inference logic.Optional: Consider minor simplification.
In
getSubLinkType(lines 528-535), you could callgetDescribedSelectStmtdirectly instead of wrapping the SelectStmt in a Node and routing throughgetDescribedNode. However, routing through the dispatcher maintains consistency with the existing pattern, so this is purely a stylistic suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/wet-ears-cross.md(1 hunks)packages/generate/src/ast-describe.ts(7 hunks)packages/generate/src/generate.test.ts(1 hunks)packages/generate/src/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/generate/src/generate.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/generate/src/ast-describe.ts (4)
packages/generate/src/utils/get-relations-with-joins.ts (3)
FlattenedRelationWithJoins(74-79)flattenRelationsWithJoinsMap(81-93)getRelationsWithJoins(11-27)packages/generate/src/utils/get-nonnullable-columns.ts (1)
getNonNullableColumns(325-333)packages/generate/src/ast-get-sources.ts (1)
getSources(42-446)packages/ast-types/src/index.ts (2)
SelectStmt(887-908)ParseResult(4-7)
🪛 GitHub Actions: CI
packages/generate/src/ast-describe.ts
[error] 1-1: Prettier formatting check failed for this file. Code style issues found. Run 'prettier --write' to fix.
🔇 Additional comments (8)
.changeset/wet-ears-cross.md (1)
1-5: LGTM!The changeset entry correctly documents the patch-level fix for scalar subquery type inference.
packages/generate/src/generate.ts (3)
19-19: LGTM!The simplified import reflects the architectural improvement—relations and non-nullable column derivation now happen inside
getASTDescription.
261-284: LGTM!The code correctly adapts to the new AST description structure:
- Accesses column descriptions via
.map.get(position)- Retrieves non-nullable metadata from
.meta.nonNullableColumns
287-298: LGTM!The context now correctly sources
relationsWithJoinsfrom the AST description metadata, eliminating redundant computation.packages/generate/src/ast-describe.ts (4)
13-18: LGTM!The new imports support centralizing relations and non-nullable column computation within the AST description flow.
51-59: LGTM! Public API restructured to include metadata.The new return shape
{ map, meta }is a breaking change but provides a cleaner API by co-locating relations and non-nullable column metadata with the per-column descriptions. The caller ingenerate.tshas been updated consistently.
72-104: LGTM! Metadata computation properly centralized.Computing
nonNullableColumnsandrelationsdirectly from the parsed AST ensures that each invocation (including recursive subquery descriptions) derives accurate metadata. This addresses the previous review concern about losing non-nullability information in scalar subqueries.
278-280: LGTM! Enables recursive subquery description.Routing
SelectStmtnodes throughgetDescribedNodeto the newgetDescribedSelectStmthandler enables proper type inference for scalar subqueries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/generate/src/ast-describe.ts (1)
521-549: Consider nullability for scalar subqueries that may return zero rows.The
getSubLinkTypehelper correctly handlesEXISTS_SUBLINK(bool) andEXPR_SUBLINK(extracts subquery type). However, passingnullable: falseat line 548 means scalar subqueries preserve the inner column's nullability without accounting for the zero-row case.In PostgreSQL, scalar subqueries return
NULLwhen they produce zero rows. For example:
(SELECT count(1) FROM tbl)→ always returns a row (correct as non-null if count is marked non-null)(SELECT col FROM tbl WHERE false)→ returns NULL (but current code infers as non-null if col is NOT NULL)The current approach correctly handles aggregates without GROUP BY (which always return one row) but may under-estimate nullability for other queries. Fixing this fully would require analyzing whether the subquery is guaranteed to return exactly one row, which is complex.
For now, this is an acceptable approximation that handles the most common cases (like
count), but consider enhancing nullability analysis for non-aggregate scalar subqueries in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/generate/src/ast-describe.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/generate/src/ast-describe.ts (4)
packages/generate/src/utils/get-relations-with-joins.ts (3)
FlattenedRelationWithJoins(74-79)flattenRelationsWithJoinsMap(81-93)getRelationsWithJoins(11-27)packages/generate/src/utils/get-nonnullable-columns.ts (1)
getNonNullableColumns(325-333)packages/generate/src/ast-get-sources.ts (1)
getSources(42-446)packages/ast-types/src/index.ts (2)
SelectStmt(887-908)ParseResult(4-7)
🔇 Additional comments (6)
packages/generate/src/ast-describe.ts (6)
51-68: LGTM - Structured return enhances API clarity.The change from returning a plain
Mapto returning{ map, meta }improves the API by making metadata (relations, nonNullableColumns) first-class. The early-return case correctly provides empty defaults for all fields. Based on the AI summary, downstream consumers ingenerate.tshave been updated accordingly.
70-72: Good approach - precompute metadata once.Computing
nonNullableColumnsandrelationsupfront and reusing them throughout the description process is efficient and ensures consistent metadata across the AST traversal.
93-103: Context properly initialized with metadata.The computed
nonNullableColumnsandrelationsare correctly threaded through the context and passed togetSources, enabling proper nullability and join-type resolution throughout the description process.
199-205: Return statement matches updated API.The return value correctly provides both the column map and the metadata, consistent with the new function signature.
276-278: Good - route SelectStmt to dedicated handler.Adding this routing enables proper description of scalar subqueries that appear directly in the select list, addressing the PR's core objective.
555-587: Well-structured recursive subquery description.The
getDescribedSelectStmtfunction properly handles nested SelectStmt nodes by:
- Constructing a synthetic
ParseResultwrapper- Recursively invoking
getASTDescription, which independently computesnonNullableColumnsviagetNonNullableColumns(subParsed)(line 70)- Extracting the first column's type for scalar subquery inference
This approach correctly addresses the previous review concern about preserving non-nullability metadata - instead of passing stale metadata, each subquery gets fresh analysis via
getNonNullableColumns.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Breaking Changes