fix(optimizer): expand positional GROUP BY in canonicalize_internal_names

[fivetran-kwoodbeck]

For BigQuery, qualify leaves a positional GROUP BY (e.g. GROUP BY 1) unexpanded when a projection alias shadows a source name. canonicalize_internal_names then renames the source to _tN, removing the collision, but never expanded the ordinal, meaning the canonical form keeps a positional reference.

canonicalize_internal_names now calls _expand_group_by after renaming. There's no dialect available (and not technically needed), so I made that parameter optional in qualify_columns.

Input

SELECT a AS foo, b AS x FROM x GROUP BY 1

Before

SELECT `_t0`.`a` AS `foo`, `_t0`.`b` AS `x` FROM `c`.`db`.`x` AS `_t0` GROUP BY 1

After

SELECT `_t0`.`a` AS `foo`, `_t0`.`b` AS `x` FROM `c`.`db`.`x` AS `_t0` GROUP BY `_t0`.`a`

[github-actions]

SQLGlot Integration Test Results

✅ All tests passed

Comparing:

• this branch (sqlglot:optimizer/fix-unstable-group-by-ordinal @ sqlglot 61b9899)
• baseline (main @ sqlglot 7784209)

Overall

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

sqlglot:optimizer/fix-unstable-group-by-ordinal: 180217 total, 142403 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]

@fivetran-kwoodbeck why is it a problem that the positional reference is not expanded? Does it mess with idempotency, or is this just an "aesthetics"-related fix?

[fivetran-kwoodbeck]

@fivetran-kwoodbeck why is it a problem that the positional reference is not expanded? Does it mess with idempotency, or is this just an "aesthetics"-related fix?

It's non idempotent, but I figured it's a canonicalization bug too in that positional references are expanded in all other cases.

[georgesittas]

This doesn't look right. In my earlier comment, I meant we needed to pass the actual source dialect corresponding to the query being transformed, so that the corresponding logic in expand_group_by could kick in.

The assumption "source already renamed, so dialect rules no longer apply" is not generally accurate. Check this BigQuery edge case, for example:

-- input
SELECT a AS _t0, b AS x FROM x GROUP BY 1

-- qualified
SELECT x.a AS _t0, x.b AS x FROM c.db.x AS x GROUP BY 1

-- canonicalized, there's a clash between the _t0 source alias & projection name
SELECT _t0.a AS _t0, _t0.b AS x FROM c.db.x AS _t0 GROUP BY _t0.a

[georgesittas]

Btw, it seems like the PR would still only partially solve the issue of idempotency. The qualify rule makes three decisions based on "a projection alias matches a source name":

1. keep GROUP BY 1 (SELECT a AS foo, b AS x FROM x GROUP BY 1)
2. demote a column to a plain identifier (SELECT a AS foo, b AS x FROM x GROUP BY a)
3. don't inline a column in a HAVING clause (SELECT a, MAX(b) AS x FROM x GROUP BY 1 HAVING x > 1)

The canonicalization rule renames the table but keeps the projection alias, so the equality breaks and all three decisions (currently) change if we apply the rules again.

The PR fixes (1), because in that case we simply copy an expression from the projection list over to the Group node. The other two, though, need a schema to redo properly, because we need to resolve the columns again, which overcomplicates the canonicalization pass. This is why I was trying to "push" this work down to qualify when we chatted about it: I don't like how we mix concerns here.

Btw, even if we do pass the right dialect here to fix (1), it seems like there's a caching issue with Scope where expand_group_by sees the scope before the rename happens, so it still sees ambiguity and doesn't do what we want it to. It'd need a rename_source followed by a clear_cache. More complexity...

Given the above, I don't see a relatively simple way forward. I don't want to overcomplicate these rules to handle a couple of edge cases related to idempotency, which is already a "good to have property" and not immediately critical (i.e., incorrect results post-transformation).

Shall we just close the PR? Do you see any reasonable alternatives?

[geooo109]

Nice analysis @georgesittas :), I prefer this to be closed for now, blast radius of the change expands in various parts + I think partially solving this isn't worth.