fix(optimizer): prevent fabrication of struct-field refs for schema-less correlated columns

[fivetran-kwoodbeck]

When qualifying a correlated subquery without a schema, qualify rewrote an unqualified column into a reference to a column that does not exist, producing an invalid query that fails to execute.

The subquery scope resolves correctly. But external_columns is computed before _qualify_columns runs, so the unqualified id gets cached as an external column. After it's qualified in place, that stale cache leaks up to the parent scope, whose _convert_columns_to_dots then rewrites it (to an invalid identifier)

Input Query:

SELECT id FROM t WHERE id > (SELECT AVG(id) FROM u WHERE u.name = t.name)

Previous Output: Note, t.u.id does not exist.

SELECT "t"."id" AS "id" FROM "t" AS "t"
WHERE "t"."id" > (SELECT AVG("t"."u"."id") AS "_col_0" FROM "u" AS "u" WHERE "u"."name" = "t"."name")

Correct Output:

SELECT "t"."id" AS "id" FROM "t" AS "t"
WHERE "t"."id" > (SELECT AVG("u"."id") AS "_col_0" FROM "u" AS "u" WHERE "u"."name" = "t"."name")

[github-actions]

SQLGlot Integration Test Results

✅ All tests passed

Comparing:

• this branch (sqlglot:optimizer/fix-qualify-correlated-column-invalid-ref @ sqlglot 76f779a)
• baseline (main @ sqlglot 2bea589)

Overall

main: 192411 total, 153578 passed (pass rate: 79.8%)

sqlglot:optimizer/fix-qualify-correlated-column-invalid-ref: 180217 total, 142426 passed (pass rate: 79.0%)

Transitions:
No change

Dialect pair changes: 0 previous results not found, 3 current results not found

✅ All tests passed

[georgesittas]

This doesn't look good for performance, Scope.columns is not trivial to compute. Was this benchmarked? I'm generally skeptical of changing hot path logic for degenerate cases like "schema does not exist"; these are mostly irrelevant.

---

@tobymao more generally (we can discuss this on Slack): is there a reason for keeping this "no schema" mode? I'm not sure if this is helpful, it's very common for queries to be ambiguous without a schema present, so this mode feels generally unreliable and we always suggest using a schema, anyway.

Things would be easier & the behavior more consistent, if we simply required the schema to be present... Without that guarantee, we increase the surface of where things could go wrong, and thus there's more maintenance overhead, i.e., the case this PR tries to solve.

[georgesittas]

Ok, I dug a bit deeper into this and found the following:

1. The performance impact is smaller than I thought (~negligible), because none of the steps that run after the eviction reads the caches. The cost is limited to parent scopes trying to pull the columns via external_columns, which happens lazily, anyway.

2. More importantly, though, the bug can also occur with a valid schema:

/* Given the following schema:

schema = {
    "t": {"id": "int", "name": "text", "u": "struct(id int)"},
    "u": {"id": "int", "name": "text"},
}

And data: t = {(1,'a',{id:1}), (2,'a',{id:999})}, u = {(2,'a')}
*/

-- Executes fine in duckdb, results in 2
SELECT id FROM t WHERE id IN (SELECT id FROM u WHERE u.name = t.name)

-- This is emitted in main, results in 1
SELECT "t"."id" AS "id" FROM "t" AS "t" WHERE "t"."id" IN (SELECT "t"."u"."id" AS "id" FROM "u" AS "u" WHERE "u"."name" = "t"."name")

So fixing this is a good call and we should make sure to test this ^ as well.

[fivetran-kwoodbeck]

That all sounds good, I added the test.

[georgesittas]

Changing my original assessment after investigating. Looks good, just needs a test for the case where we have a valid schema.

[georgesittas]

Thanks Kris 👍