TypeCheck now can preserve AnnotateTypeExpr to prevent invalid re-typing

[delphamk]

Only discard AnnotateTypeExpr when its type is redundant.

Example: 'min(NULL:::FLOAT)' now resolves to type FLOAT instead of STRING.

Epic: none

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[mw5h]

Review

The core idea is sound — preserving type annotations on NULL to prevent downstream type inference from losing the user's intent. However, there are a few issues to address.

1. Hand-edited generated file (blocking)

pkg/sql/sem/tree/eval_expr_generated.go is explicitly marked:

// Code generated by eval_gen.go. DO NOT EDIT.
// Regenerate this file with the command:
//   ./dev generate go

The PR hand-edits this file to add EvalAnnotateTypeExpr to the ExprEvaluator interface and the Eval method on *AnnotateTypeExpr. The correct approach is:

1. Add the typeAnnotation embedding to AnnotateTypeExpr in expr.go (which the PR does)
2. Run ./dev generate go to regenerate eval_expr_generated.go

The hand-edits will likely cause CI failures or be overwritten.

2. Logic condition in TypeCheck may be fragile

The new condition at type_check.go:817:

if subExpr.ResolvedType() != types.Unknown || annotateType.IsWildcardType() || cast.ValidCast(annotateType, desired, cast.ContextImplicit) {
    return subExpr, nil
}

The ValidCast(annotateType, desired, ...) check uses the desired type, which is the type the caller wants (e.g., what an aggregate function expects). This means the behavior of ::: depends on surrounding context, which seems fragile. Consider: what if desired is types.Any? ValidCast to Any may always succeed, stripping the annotation even when it should be preserved.

A clearer condition might be: "preserve the annotation only when the inner expression is Unknown and the annotated type differs from Unknown."

3. buildScalar handling may be too narrow

In scalar.go, the new case only preserves type for DNull:

case *tree.AnnotateTypeExpr:
    texpr := t.TypedInnerExpr()
    if texpr == tree.DNull {
        out = b.factory.ConstructConstVal(tree.DNull, t.ResolvedType())
    } else {
        return b.buildScalar(texpr, inScope, outScope, outCol, colRefs)
    }

Could there be cases where the wrapper is preserved but the inner expression isn't literally DNull? For example, an expression that resolves to Unknown type but isn't the DNull constant. In that case, the else branch would silently discard the annotation.

4. Missing broader test coverage

The tests cover the optimizer builder (build testdata), but there are no end-to-end SQL tests verifying the actual query result. For instance, a logic test confirming:

SELECT pg_typeof(min(NULL:::FLOAT))  -- should return 'double precision'

This would catch regressions at the execution level, not just the plan level.

5. nit: comment style

// return subExpr if the AnnotateTypeExpr is redundant

Should be a complete sentence per Go conventions: // Return subExpr if the AnnotateTypeExpr is redundant.

[mw5h]

Turns out even AI can double-post.

This patch is an interesting idea, but it's going to need a little more work before it's ready to merge.

[delphamk]

I appreciate your comments. I’ve provided an explanation of my reasoning for the TypeCheck change and hope it’s understandable.

1) Fixed and caused by a formatting issue in pkg/sql/sem/eval/expr.go
2) TypeCheck logic

1. subExpr.ResolvedType() != types.Unknown

• subExpr is Unknown and 'This type should never be returned for an expression that does not *always* \ evaluate to NULL.'
• This means duplicate calls to subExpr.TypeCheck(...) will resolve to annotateType at a minimum. Implicit casting could change its type, but that is valid.
• It is still possible for the annotation to be striped when subExpr is Unknown and is covered in the following or checks.

2. annotateType.IsWildcardType()

• This can happen for NULL:::RECORD. When the annotation is preserved the otherwise NULL value is not reTyped to a tuple as it would normally. I think that AnyTuple requires some changes for this. For example SELECT CASE WHEN true THEN ('a', 2) ELSE NULL::RECORD END will error, where NULL does not.
• A better check here may be annotateType.IsAmbiguous because "IsAmbiguous returns true if this type is in UnknownFamily or AnyFamily."
• In other words, this checks if the the annotation allows NULL/Unknown, and if true, the subExpr has resolved as expected.

3. ValidCast(annotateType, desired, ...)

• This is intended to cover cases like NULL:::INT:::FLOAT , where :::INT can be discarded since upstream implicitly desires a FLOAT.

what if desired is types.Any?

• ValidCast will return false unless if annotateType is also types.Any. This may be a confusion with if desired is types.Unknown, which always return true.

Simply, this change is looking to fix cases like min(NULL:::FLOAT).
The subExpr (stripped to NULL) will be cast to String as a way to reduce the number of overloads.
Considering that Annotations have been returning the subExpr up to this point, the logic could be narrowed down to: !(subExpr.ResolvedType() != types.Unknown && desired != types.AnyElement) without breaking existing logic.

3) buildScalar logic
For this the only cases to slip though would be types.Unknown and !IsWildcardType.
I updated the logic to check unknown type instead and then cast to its type.

4) Added some more cases but can add more if needed.
For test case you mentioned, currently it is unknown which is the same as SELECT pg_typeof(min(NULL::FLOAT));

5) Updated the comment but can make it more verbose if needed.

Regarding my comments on RECORD casting, I have an example of what this might look like here. With this change, the IsAmbiguous should not be needed.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[delphamk]

Pushed an updated revision with rebase.

Please let me know if there are any other comments. I plan on using this commit in another PR and will reference this for context.

I agree that the check for 2) does not cover the full condition but it still tightens NULL annotations. As I mentioned, NULL:::RECORD is not covered yet. This change instead covers most missing NULL annotations without reducing previous cases.