Skip to content

fix(optimizer): expand positional GROUP BY in canonicalize_internal_names #8005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fivetran-kwoodbeck wants to merge 3 commits into from
+36 −3
Closed

fix(optimizer): expand positional GROUP BY in canonicalize_internal_names #8005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f238bff
add _expand_group_by to canonicalize
fivetran-kwoodbeck Jul 30, 2026
e6dfd76
expand_group_by updates
fivetran-kwoodbeck Jul 31, 2026
66576d3
missing import
fivetran-kwoodbeck Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions sqlglot/optimizer/canonicalize_internal_names.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,19 @@
import typing as t

from sqlglot import exp
from sqlglot.dialects.dialect import Dialect
from sqlglot.helper import name_sequence, seq_get
from sqlglot.optimizer.qualify_columns import expand_group_by
from sqlglot.optimizer.scope import Scope, find_all_in_scope, traverse_scope

if t.TYPE_CHECKING:
from sqlglot._typing import E

# canonicalize_internal_names is dialect-agnostic: the source has already been
# renamed so dialect specific rules (e.g. PROJECTION_ALIASES_SHADOW_SOURCE_NAMES) no
# longer apply.
_DEFAULT_DIALECT = Dialect()
Comment on lines +14 to +17

@georgesittas georgesittas Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 georgesittas Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 geooo109 Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def canonicalize_internal_names(expression: E) -> E:
"""
Expand Down Expand Up @@ -287,6 +294,9 @@ def _canon(ident: exp.Identifier, name: str) -> None:
if not col.table and col.name in output_map:
_canon(col.this, output_map[col.name])

# Expand positional GROUP BYs (excluded in qualify) now that the source is renamed
expand_group_by(scope, _DEFAULT_DIALECT)

# UBN matches branches by original alias. When both branches are internal
# and aliased to distinct _cN, matching originals land on different slots
# and UBN splits them into disjoint output columns, dropping data. Align
Expand Down
9 changes: 6 additions & 3 deletions sqlglot/optimizer/qualify_columns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def qualify_columns(
)
qualify_outputs(scope, dialect=dialect)

_expand_group_by(scope, dialect)
expand_group_by(scope, dialect)

# DISTINCT ON and ORDER BY follow the same rules (tested in DuckDB, Postgres, ClickHouse)
# https://www.postgresql.org/docs/current/sql-select.html#SQL-DISTINCT
Expand Down Expand Up @@ -450,7 +450,7 @@ def replace_columns(
scope.clear_cache()


def _expand_group_by(scope: Scope, dialect: Dialect) -> None:
def expand_group_by(scope: Scope, dialect: Dialect) -> None:
expression = scope.expression
group = expression.args.get("group")
if not group:
Expand Down Expand Up @@ -506,7 +506,10 @@ def _expand_order_by_and_distinct_on(scope: Scope, resolver: Resolver) -> None:


def _expand_positional_references(
scope: Scope, expressions: Iterable[exp.Expr], dialect: Dialect, alias: bool = False
scope: Scope,
expressions: Iterable[exp.Expr],
dialect: Dialect,
alias: bool = False,
) -> list[exp.Expr]:
new_nodes: list[exp.Expr] = []
ambiguous_projections = None
Expand Down
20 changes: 20 additions & 0 deletions tests/fixtures/optimizer/canonicalize_internal_names.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,3 +277,23 @@ WITH "_t1" AS (SELECT "_t0"."a" + 1 AS "_c0" FROM "c"."db"."x" AS "_t0") SELECT
# title: ORDER BY reference to a CTE-level alias follows the alias's _cN canonicalization
WITH t AS (SELECT a + 1 AS total FROM x ORDER BY total) SELECT total FROM t;
WITH "_t1" AS (SELECT "_t0"."a" + 1 AS "_c0" FROM "c"."db"."x" AS "_t0" ORDER BY "_c0") SELECT "_t1"."_c0" AS "total" FROM "_t1" AS "_t1";

# title: GROUP BY ordinal is expanded even when a projection alias shadows the pre-canonicalization source name, so the form is stable once the source is renamed to _t0
# dialect: bigquery
SELECT a AS foo, b AS x FROM x GROUP BY 1;
SELECT `_t0`.`a` AS `foo`, `_t0`.`b` AS `x` FROM `c`.`db`.`x` AS `_t0` GROUP BY `_t0`.`a`;

# title: GROUP BY ordinal is expanded when an aggregate projection alias shadows the source name
# dialect: bigquery
SELECT a AS foo, SUM(b) AS x FROM x GROUP BY 1;
SELECT `_t0`.`a` AS `foo`, SUM(`_t0`.`b`) AS `x` FROM `c`.`db`.`x` AS `_t0` GROUP BY `_t0`.`a`;

# title: GROUP BY ordinal is expanded inside a CTE scope where a projection alias shadows the source name
# dialect: bigquery
WITH t AS (SELECT a AS foo, b AS x FROM x GROUP BY 1) SELECT * FROM t;
WITH `_t1` AS (SELECT `_t0`.`a` AS `_c0`, `_t0`.`b` AS `_c1` FROM `c`.`db`.`x` AS `_t0` GROUP BY `_t0`.`a`) SELECT `_t1`.`_c0` AS `foo`, `_t1`.`_c1` AS `x` FROM `_t1` AS `_t1`;

# title: GROUP BY ordinals both expand once a self-join alias that shadowed a projection alias is renamed
# dialect: bigquery
SELECT x.a AS c, y.a AS d, SUM(x.b) AS y FROM x AS x JOIN x AS y ON x.a = y.a GROUP BY 1, 2;
SELECT `_t0`.`a` AS `c`, `_t1`.`a` AS `d`, SUM(`_t0`.`b`) AS `y` FROM `c`.`db`.`x` AS `_t0` JOIN `c`.`db`.`x` AS `_t1` ON `_t0`.`a` = `_t1`.`a` GROUP BY `_t0`.`a`, `_t1`.`a`;
Loading