Skip to content

fix(annotate_types)!: propagate type through IgnoreNulls/RespectNulls wrappers [CLAUDE]#7636

Merged
georgesittas merged 2 commits into
tobymao:mainfrom
RichardHughes-amp:main
May 12, 2026
Merged

fix(annotate_types)!: propagate type through IgnoreNulls/RespectNulls wrappers [CLAUDE]#7636
georgesittas merged 2 commits into
tobymao:mainfrom
RichardHughes-amp:main

Conversation

@RichardHughes-amp
Copy link
Copy Markdown
Contributor

@RichardHughes-amp RichardHughes-amp commented May 11, 2026

Added unary type annotation to exp.IgnoreNulls and exp.RespectNulls.

Summary

FIRST_VALUE(expr, true) gets parsed into IgnoreNulls(FirstValue(expr)), blocking type annotation until IgnoreNulls and RespectNulls are annotated.

Test plan

Fixture cases to tests/fixtures/optimizer/annotate_functions.sql covering FIRST_VALUE(col) IGNORE NULLS and LAST_VALUE(col) RESPECT NULLS across six dialects: Spark, Databricks, Snowflake, BigQuery, Trino, and Redshift

…type propagation

IGNORE NULLS and RESPECT NULLS wrappers should propagate their inner
expression's type, but the TypeAnnotator has no handler for them so
they return UNKNOWN even when the inner expression is correctly typed.

[CLAUDE]

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@RichardHughes-amp RichardHughes-amp force-pushed the main branch 2 times, most recently from ff1e3b6 to 5bef120 Compare May 11, 2026 20:20
@RichardHughes-amp RichardHughes-amp marked this pull request as ready for review May 11, 2026 20:24
Copy link
Copy Markdown
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Introducing a new class just to have a common base seems unnecessary. Please refactor this so that IgnoreNulls and RespectNulls are inlined in its place.

Another piece of feedback: including Claude's PR description verbatim produces, imo, more noise than value. It'd be much better if you distilled the necessary information and communicated it succinctly.

@RichardHughes-amp
Copy link
Copy Markdown
Contributor Author

RichardHughes-amp commented May 12, 2026

Introducing a new class just to have a common base seems unnecessary. Please refactor this so that IgnoreNulls and RespectNulls are inlined in its place.

Can do. (This one is my fault, ha ha - Claude originally did this but I thought it was visually busy enough that I went looking for something more 'elegant'.)

Another piece of feedback: including Claude's PR description verbatim produces, imo, more noise than value. It'd be much better if you distilled the necessary information and communicated it succinctly.

I'll see if I can breath some life in to the withered nubbin of a temporal lobe to summarize the hideous mutterings of the linear algebra demon.

…wrappers

Introduce NullTreatment as a common base class for IgnoreNulls and
RespectNulls, then include it alongside Unary/Alias in the unary
annotator registration. Both wrappers carry a single `this` child and
never change the expression's type — only its null-handling behaviour —
so propagating `.this.type` is the correct semantic.

[CLAUDE]

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@georgesittas georgesittas changed the title fix(annotate_types): propagate type through IgnoreNulls/RespectNulls wrappers [CLAUDE] fix(annotate_types)!: propagate type through IgnoreNulls/RespectNulls wrappers [CLAUDE] May 12, 2026
@georgesittas georgesittas merged commit c6615a9 into tobymao:main May 12, 2026
10 checks passed
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.

2 participants