Keep table qualifiers for sql fields referencing other tables#5795
Open
ATOM00blue wants to merge 1 commit into
Open
Keep table qualifiers for sql fields referencing other tables#5795ATOM00blue wants to merge 1 commit into
ATOM00blue wants to merge 1 commit into
Conversation
In a single-table select, buildSelection strips the table prefix from every column in a custom `sql` field. That is fine for a bare reference to the queried table, but it also rewrites columns inside a correlated subquery written in the field, so `b.c_id = a.c_id` becomes the always-true `c_id = c_id` and the query silently returns wrong results. The same expression renders correctly in `where`. Only drop the prefix when the expression does not pull in another table (`usedTables` is empty). Applied to pg, mysql, sqlite and singlestore. Fixes drizzle-team#5734
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adjusts dialect query building to avoid stripping table qualifiers inside SQL expressions when building single-table selects, and adds integration tests to cover correlated-subquery scenarios.
Changes:
- Update SQL field handling in SQLite/Pg/MySQL/SingleStore dialects to only drop table prefixes when the SQL expression does not reference any tables.
- Add integration tests for SQLite/Pg/MySQL asserting table qualifiers are preserved in a correlated subquery embedded in a select field.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/tests/sqlite/sqlite-common.ts | Adds a regression test for correlated subquery qualifier preservation in sql select fields. |
| integration-tests/tests/pg/pg-common.ts | Adds the same regression test for Postgres. |
| integration-tests/tests/mysql/mysql-common.ts | Adds the same regression test for MySQL. |
| drizzle-orm/src/sqlite-core/dialect.ts | Changes when table prefixes are dropped for SQL/SQL.Aliased fields in single-table selects. |
| drizzle-orm/src/pg-core/dialect.ts | Same logic change for Postgres dialect. |
| drizzle-orm/src/mysql-core/dialect.ts | Same logic change for MySQL dialect. |
| drizzle-orm/src/singlestore-core/dialect.ts | Same logic change for SingleStore dialect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1132
to
+1138
| label: sql`select ${b.label} from ${b} where ${b.cId} = ${a.cId} limit 1`, | ||
| }) | ||
| .from(a) | ||
| .toSQL(); | ||
|
|
||
| expect(query).toEqual({ | ||
| sql: 'select "id", select "b"."label" from "b" where "b"."c_id" = "a"."c_id" limit 1 from "a"', |
Comment on lines
+1173
to
+1179
| label: sql`select ${b.label} from ${b} where ${b.cId} = ${a.cId} limit 1`, | ||
| }) | ||
| .from(a) | ||
| .toSQL(); | ||
|
|
||
| expect(query).toEqual({ | ||
| sql: 'select "id", select "b"."label" from "b" where "b"."c_id" = "a"."c_id" limit 1 from "a"', |
Comment on lines
+1158
to
+1164
| label: sql`select ${b.label} from ${b} where ${b.cId} = ${a.cId} limit 1`, | ||
| }) | ||
| .from(a) | ||
| .toSQL(); | ||
|
|
||
| expect(query).toEqual({ | ||
| sql: 'select `id`, select `b`.`label` from `b` where `b`.`c_id` = `a`.`c_id` limit 1 from `a`', |
Comment on lines
+192
to
+194
| // Drop the table prefix for bare column refs in a single-table select, but keep it | ||
| // when the expression references another table (e.g. a correlated subquery). | ||
| if (isSingleTable && query.usedTables.length === 0) { |
Comment on lines
+228
to
+230
| // Drop the table prefix for bare column refs in a single-table select, but keep it | ||
| // when the expression references another table (e.g. a correlated subquery). | ||
| if (isSingleTable && query.usedTables.length === 0) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5734
Problem
When a raw
sqlexpression is used as a selected field in a single-tableselect, Drizzle strips the table qualifier from every column inside that
expression. For a bare reference to the queried table that is harmless,
but it also rewrites columns that belong to another table — e.g. a
correlated subquery written directly in the field list. The condition
is emitted as
which is always true, so the query compiles and runs but silently returns
wrong results. The same
sqlchunk renders correctly when used in.where(...).Root cause
buildSelectionapplies the single-table optimization by walking theexpression's top-level
queryChunksand replacing every column chunkwith an unqualified identifier. When a column written inline references a
different table, that qualifier is required and must not be dropped.
Fix
Only strip the qualifier when the expression does not reference another
table, i.e. its
usedTablesis empty. A bare same-table column referencekeeps the optimization (
select id from a), while an expression thatpulls in another table is left untouched and renders with qualifiers,
matching how the same chunk already renders in
where.The same buggy branch existed in the pg, mysql, sqlite and singlestore
dialects, so the change is applied to all four. gel-core already disables
this optimization and is unaffected.
Tests
Added a
build queryregression test to the pg, mysql and sqlite commonsuites asserting that a correlated subquery in a single-table select's
field list keeps its table qualifiers. The test fails before the change
and passes after it. The existing
build querytest (which covers theplain-column optimization) still passes.