Skip to content

fix(optimizer): type nested set operations pairwise and by position - #8702

Closed
treysp wants to merge 2 commits into
mainfrom
trey/nested-setops-types
Closed

treysp wants to merge 2 commits into
mainfrom
trey/nested-setops-types

Conversation

@treysp

@treysp treysp commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Currently, types for nested set operations are taken from the leftmost query and merged by name, so types in the nested operation are not properly bubbled up.

For example, in this query annotate_types infers t.a as INT, while Postgres (double precision) and DuckDB (DOUBLE) both report a double:

SELECT 
  t.a 
FROM (
  SELECT 
    1 AS a 
  UNION ALL (
    SELECT 
      2 AS b 
    UNION ALL 
    SELECT 
      2.5::DOUBLE AS c
  )
) AS t

Cause

  • Each set operation compares this.selects with expression.selects. For a parenthesized operand, Subquery.selects returns only the projections of its first query (SELECT 2 AS b). The outer UNION therefore never sees the DOUBLE.
  • Every set operation in the tree writes its result into one shared dict, keyed by its own left query's column names. The inner UNION computes DOUBLE correctly, but stores it under b, which the outer column a never reads.

Left-nested chains currently work only because every operation's left query is the same first query, so the names always line up.

Fix

Resolve set operations pairwise and bottom-up:

  • Set operations are resolved in reverse walk() order, so nested operations resolve before the ones containing them.
  • A parenthesized operand contributes its own resolved columns rather than its first query's selects.
  • Columns match by position (by name for BY NAME), and output names come from the left operand.
  • A column-count mismatch inside the tree now gives anUNKNOWN type rather than the first query's type.

This comment was marked as resolved.

@github-actions

This comment was marked as outdated.

@treysp
treysp force-pushed the trey/nested-setops-types branch from e2b265a to 1ca6b68 Compare October 20, 2026 19:51
@treysp
treysp requested a balanced review from Copilot October 20, 2026 19:52

This comment was marked as outdated.

setop_cols = {
ls.alias_or_name: self._maybe_coerce(
t.cast(exp.DataType, ls.type), t.cast(exp.DataType, rs.type)
for node in reversed(set_ops):

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 isn't this PR's fault, but I'm not a fan of this approach. I understand that we do it because set operation type inference is deferred until someone asks for it via _get_setop_column_types, but this ends up introducing more complexity for no good reason.

I don't remember why we chose to annotate these set operations lazily to begin with, but I think we can probably refactor that. So: annotate set operation branches as you walk up the scopes in post-order and then simplify this by only running the coercions between branches etc.

Maybe I'll take a stab at this separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants