Skip to content

Commit 6ca7492

Browse files
committed
docs(analyzer): shorten TODO comments
Refs: #1648
1 parent e953257 commit 6ca7492

2 files changed

Lines changed: 8 additions & 33 deletions

File tree

server/analyzer/resolve_values_types.go

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -131,31 +131,10 @@ func ResolveValuesTypes(ctx *sql.Context, a *analyzer.Analyzer, node sql.Node, s
131131
for _, child := range n.Children() {
132132
childSchema = append(childSchema, child.Schema()...)
133133
}
134-
// TODO: resolve GMS case asymmetry issues.
135-
// GMS has a casing asymmetry for aggregate names that forces
136-
// case-insensitive matching here. GMS's Builder.buildAggregateFunc()
137-
// in planbuilder/aggregates.go lowercases the entire aggregate
138-
// name producing "sum(v.n)", but GroupBy.Schema() in
139-
// plan/group_by.go keeps original casing from e.String()
140-
// producing "SUM(v.n)". Without strings.ToLower, the match
141-
// fails silently and aggregate type propagation breaks, causing
142-
// runtime panics (interface conversion: interface {} is
143-
// decimal.Decimal, not int32).
144-
//
145-
// We can't use non-name matching because sql.Column has no
146-
// ColumnId field, so there is nothing on the child schema side
147-
// to match against GetField.Id(). Name is the only shared
148-
// identifier.
149-
//
150-
// This causes a known false-match when two quoted column names
151-
// differ only by case (e.g., "Val" vs "val"), since the
152-
// planbuilder has already lowered both to the same GetField
153-
// name. GMS originated as a MySQL engine where identifiers are
154-
// case-insensitive, but Postgres requires case-sensitivity for
155-
// quoted identifiers. A proper fix requires structured
156-
// case-sensitivity discrimination in GMS, either by adding
157-
// ColumnId to sql.Column or by fixing the casing asymmetry in
158-
// Builder.buildAggregateFunc() and GroupBy.Schema().
134+
// TODO: GMS is case-insensitive for identifiers, so aggregate
135+
// GetField names and child schema names may differ in casing.
136+
// We use strings.ToLower to handle this, but Postgres requires
137+
// case-sensitivity for quoted identifiers, which this breaks.
159138
gfName := strings.ToLower(gf.Name())
160139
for _, col := range childSchema {
161140
if strings.ToLower(col.Name) == gfName && gf.Type() != col.Type {

testing/go/values_statement_test.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -558,14 +558,10 @@ var ValuesStatementTests = []ScriptTest{
558558
Name: "VALUES with case-differing quoted columns and aggregates",
559559
Assertions: []ScriptTestAssertion{
560560
{
561-
// TODO: resolve GMS case asymmetry issues.
562-
// Builder.buildAggregateFunc() in planbuilder/aggregates.go
563-
// lowercases entire aggregate names, so SUM("Val") and
564-
// SUM("val") both become "sum(v.val)" in the GetField. The
565-
// second pass in ResolveValuesTypes must use strings.ToLower
566-
// to match against GroupBy.Schema() which keeps original
567-
// casing. This causes a false match when two columns differ
568-
// only by case. See resolve_values_types.go for details.
561+
// TODO: GMS is case-insensitive for identifiers, but
562+
// Postgres requires case-sensitivity for quoted identifiers.
563+
// Columns "Val" and "val" both become "val" after
564+
// strings.ToLower, so their aggregates match the wrong column.
569565
Skip: true,
570566
Query: `SELECT SUM("Val"), SUM("val") FROM (VALUES(1, 10),(2.5, 20)) v("Val", "val");`,
571567
Expected: []sql.Row{

0 commit comments

Comments
 (0)