google / google/googlesql

RET_CHECK when casting a range variable to a struct through a no-op cast

Open
#172 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2.6k
Forks
260
PR merge metrics
No merged PRs in 30d

Description

The analyzer fails with an internal `RET_CHECK` instead of resolving, when a range variable is cast to a struct type and an intervening cast is eliminated as a no-op.

Reproduced on `fd972655db97deac02f0696ea652a390209b794b`.
Also reproduced with the official GoogleSQL 2026.7.2 `execute_query` Linux
binary using `--catalog=sample --mode=analyze`; it prints the same error.

## Repro

```sql
SELECT CAST(CAST(kv AS STRUCT) AS STRUCT)
FROM KeyValue kv
```

Actual:

```
INTERNAL: GOOGLESQL_RET_CHECK failure (googlesql/analyzer/function_resolver.cc:1139)
Cannot obtain the AST expressions for field arguments of struct constructor:
CastExpression [12-55]
PathExpression [17-19]
Identifier(kv) [17-19]
...
```

Expected: the statement resolves, taking the same struct `Cast` that the bare
`CAST(kv AS STRUCT)` already takes today.

The inner cast has to be a **no-op** — its target type must equal the range
variable's own type — and the outer cast has to target a struct. If the inner
cast changes the type, or is absent, the statement resolves normally.

The same query returns `An internal error occurred and the request could not be
completed. Error: 80038528` from the BigQuery service (observed 2026-08-05), so
this is not specific to a local build configuration.

## Mechanism

`FunctionResolver::AddCastOrConvertLiteral`
(`googlesql/analyzer/function_resolver.cc:1362`) decomposes a struct cast field
by field, guarded at `:1398`:

```cpp
if (target_type->IsStruct() &&
argument->get()->node_kind() == RESOLVED_MAKE_STRUCT &&
ast_location->node_kind() != AST_PATH_EXPRESSION) {
```

and calls `ExtractStructFieldLocations` (`:1076`) immediately inside that branch,
at `:1413`.

The `AST_PATH_EXPRESSION` exclusion is there for a good reason, stated in the
comment above it: a range variable resolves to a `ResolvedMakeStruct` that no
written struct constructor stands behind, so there are no field expressions to
extract, and `ExtractStructFieldLocations`' `default:` arm is a
`GOOGLESQL_RET_CHECK_FAIL` (`:1138`).

**The two disagree about which AST node to look at.**
`ExtractStructFieldLocations` first walks down through gratuitous wrappers
(`:1081`):

```cpp
// Skip through gratuitous casts in the AST so that we can get the field
// argument locations.
const ASTNode* cast_free_ast_location = ast_location;
while (cast_free_ast_location != nullptr) {
if (cast_free_ast_location->node_kind() == AST_CAST_EXPRESSION) {
...
} else if (cast_free_ast_location->node_kind() == AST_NAMED_ARGUMENT) {
...
```

and only then switches on the node kind. The guard at `:1400` does not skip
anything — it tests `ast_location` directly.

So for the repro:

1. The inner `CAST` is a no-op, and `ResolveCastWithResolvedArgument`
(`resolver_expr.cc:7612`) drops it — `IsCastNoop` (`:7593`) returns true when
the source and target types and annotations match, and unless
`preserve_unnecessary_cast()` is set the function returns without building a
`ResolvedCast` (`:7620`).
2. The argument reaching the outer cast is therefore the range variable's
`ResolvedMakeStruct`, exactly as in the bare-path-expression case.
3. But `ast_location` is now the *inner* `ASTCastExpression`, not the
`ASTPathExpression`, so the guard's exclusion does not fire and the branch is
entered.
4. `ExtractStructFieldLocations` skips back through that same cast, arrives at
`AST_PATH_EXPRESSION`, finds no constructor, and hits its `RET_CHECK_FAIL`.

The immediate defect is inconsistent AST normalization: in this repro the guard
is one wrapper too shallow, and the elision in step 1 is what puts that wrapper
in between. More generally, the branch is selected with a partial negative test
(`not AST_PATH_EXPRESSION`), while the extractor requires a positive struct-
constructor shape.

## Suggested fix

The immediate fix is to normalize `ast_location` through the same cast and
named-argument wrappers before applying the path-expression exclusion. That
closes this repro and other wrapped-path variants: the normalized node is
`AST_PATH_EXPRESSION`, so resolution falls through to the whole-struct `Cast`.

Normalization alone does not make the guard and extractor agree on every
possible input. The guard would still admit any normalized non-path node, while
`ExtractStructFieldLocations` accepts exactly
`AST_STRUCT_CONSTRUCTOR_WITH_PARENS`, `AST_STRUCT_CONSTRUCTOR_WITH_KEYWORD`,
`AST_BRACED_CONSTRUCTOR` and `AST_STRUCT_BRACED_CONSTRUCTOR`. Any other operand
that resolves to a `ResolvedMakeStruct` would still reach the extractor's
`default:` arm.

A more robust fix is to combine recognition and extraction into one helper, for
example `TryExtractStructFieldLocations`, that:

1. unwraps gratuitous casts and named arguments;
2. returns field locations for one of the supported struct constructors; and
3. returns no locations for every other node kind.

`AddCastOrConvertLiteral` should use the field-by-field path only when that
helper returns locations; otherwise it should fall through to the whole-struct
`Cast`. This shares both wrapper normalization and the accepted-node set by
construction, while retaining `RET_CHECK`s for impossible field-count
mismatches after a constructor has been recognized.

A positive `IsSupportedStructConstructor` test on the normalized node would
also close all currently unsupported node kinds, but duplicating the constructor
list next to the extractor could allow the two to drift again.

## Suggested test cases

```sql
-- currently RET_CHECKs; should resolve
SELECT CAST(CAST(kv AS STRUCT) AS STRUCT)
FROM KeyValue kv

-- already resolves; guards the fix against regressing the bare case
SELECT CAST(kv AS STRUCT) FROM KeyValue kv

-- already resolves; the field-by-field path must stay field-by-field
SELECT CAST(STRUCT(1 AS a, 'x' AS b) AS STRUCT)

-- already resolves; non-literal fields ensure this actually exercises the
-- extractor's wrapper-skipping path and remains field-by-field
SELECT CAST(CAST((Key, Value) AS STRUCT) AS STRUCT)
FROM KeyValue
```

Contributor guide

Open the contributing guide

Research direction

Start in googlesql/analyzer/function_resolver.cc at AddCastOrConvertLiteral and ExtractStructFieldLocations, then trace no-op cast removal in resolver_expr.cc at ResolveCastWithResolvedArgument. Reproduce the query with the analyzer or execute_query, align wrapper handling between the guard and extractor, and verify all four supplied SQL cases resolve without regressing field-by-field casts.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, sql
Domain
compilers, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.