Skip to content

refactor(optimizer)!: defer scope annotation until qualification is complete - #8696

Open
georgesittas wants to merge 1 commit into
jo/annotator-05-struct-expansionfrom
jo/annotator-06-deferred-annotation
Open

georgesittas wants to merge 1 commit into
jo/annotator-05-struct-expansionfrom
jo/annotator-06-deferred-annotation

Conversation

@georgesittas

@georgesittas georgesittas commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

During qualification, annotate struct-star operands only when expansion needs their types. Resolve source dependencies iteratively so long CTE chains do not overflow. Defer whole-scope annotation until qualification is complete for dialects that require it, eliminating repeated cache invalidation.

Breaking change: removes TypeAnnotator.uncache.

This comment was marked as resolved.

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

SQLGlot Integration Test Results

✅ All tests passed

Comparing:

  • this branch (sqlglot:jo/annotator-06-deferred-annotation @ sqlglot 2862a27)
  • baseline (main @ sqlglot eb03a97)

By Dialect

dialect main feature branch transitions links
bigquery -> bigquery 34290/34695 passed (98.8%) 34297/34695 passed (98.9%) 7 fail -> pass full result / delta

Overall

main: 182929 total, 165991 passed (pass rate: 90.7%)

sqlglot:jo/annotator-06-deferred-annotation: 182929 total, 165998 passed (pass rate: 90.7%)

Transitions:
7 fail -> pass

✅ All tests passed

@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch from cb78761 to 846420d Compare October 20, 2026 15:35
@georgesittas
georgesittas requested a balanced review from Copilot October 20, 2026 15:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Lazy source annotation can exhaust Python’s call stack on long, flat CTE dependency chains.

1 open issue

Review effort: Balanced

Comment thread sqlglot/optimizer/annotate_types.py Outdated
@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch 2 times, most recently from 0a315ad to 1e17fba Compare October 20, 2026 16:23
@georgesittas
georgesittas requested a balanced review from Copilot October 20, 2026 16:24
@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch from 1e17fba to 3c375d1 Compare October 20, 2026 16:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Correlated subqueries still trigger recursive source resolution, allowing long CTE chains to overflow the call stack.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment on lines +358 to +368
dependencies = itertools.chain(
current.cte_scopes,
current.set_operation_scopes,
current.table_scopes,
current.subquery_scopes,
(
dependency
for name, _ in current.references
if isinstance(dependency := current.sources.get(name), Scope)
),
)

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 is only triggered for RisingWane. BQ uses ANNOTATE_ALL_SCOPES = TRUE so it's fine.

@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch 2 times, most recently from 4155507 to 00abb31 Compare October 20, 2026 19:43
@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch 2 times, most recently from 3556ce0 to a79afbe Compare October 21, 2026 17:57
@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch from a79afbe to 24475be Compare October 23, 2026 11:15
@georgesittas
georgesittas force-pushed the jo/annotator-06-deferred-annotation branch from 24475be to 2af3a59 Compare October 23, 2026 11:29

This branch has not been deployed

No deployments
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