refactor(firestore): fix aliasing#14440
Conversation
The ProblemInitially, the Go SDK utilized struct embedding for type AliasedExpression struct {
*baseExpression
alias string
}Because of Go's embedding rules, // Invalid logic but compiled successfully:
expr := firestore.Constant(1).As("number").Add(2)In the example above, calling Furthermore, users could chain aliases indefinitely ( The SolutionTo ensure compile-time safety and semantic clarity, the Go implementation is updated to remove struct embedding entirely.
type AliasedExpression struct {
expr Expression
alias string
}
// Does NOT implement Expression interface methods like Add(), Multiply(), or As()Benefits of this Approach:
Cross-SDK Comparison of
|
| SDK | As().As() Chaining Behavior |
Implementation Details | Intentional Design? |
|---|---|---|---|
| Go (Current) | Compile Error | AliasedExpression is strictly a Selectable and does not expose As() or any Expression methods. Limits alias to exactly 1 per expression. |
Yes. Provides maximum type-safety and prevents accidental method calls on projections. |
| Node.js | Compile/Runtime Error | AliasedExpression does not implement an as() method and isn't a child of Expression. Limits alias to exactly 1. |
Yes. Strictly enforces a single alias per expression. |
| Java | Replaces previous alias | Explicitly implements as() on AliasedExpression. It unwraps the first alias and replaces it. Limits chaining to 2 calls because the return type of the second call is a Selectable interface (which lacks an as() method). |
Yes, but slightly awkward as it requires an intermediate return type change to cap the chaining. |
| Python | Nests aliases indefinitely | AliasedExpression inherits from Expression (via Selectable). Calling as_() wraps the existing AliasedExpression recursively. This produces nested map structures in the Protocol Buffer (e.g., {"test": {"number": 1}}). |
Likely No. This seems to be an unintended side effect of inheritance, creating potentially invalid proto payloads. |
There was a problem hiding this comment.
Code Review
This pull request refactors AliasedAggregate and AliasedExpression by replacing embedded base structs with explicit interface fields. The review feedback highlights opportunities to improve consistency between these two types, specifically by ensuring AliasedAggregate stores the full AggregateFunction interface and providing a getter method for its unexported fields to match the design of AliasedExpression.
Currently, AliasedExpressions are treated like regular expressions. You
can execute additional expressions off of them
(`a.as_("number").add(5)`), or chain them
(`a.as_("first").as_("second")`). But the backend doesn't actually
support aliases being used in this way
This PR raises an exception if an alias is used in a context it doesn't
support
Go version: googleapis/google-cloud-go#14440
Redesigning aliasing