[ajitpratap0]

Summary

ExtractColumns / ExtractColumnsQualified listed a SELECT-list alias as a column whenever ORDER BY or HAVING referenced it (e.g. COUNT(o.id) AS orders ... ORDER BY orders reported orders). This also affected the MCP extract_metadata / analyze_sql tools and the ChatGPT widget.

The collector now builds each SELECT's alias set (lazily, only when the SELECT has ORDER BY/HAVING and aliases) and skips unqualified matches in that SELECT's ORDER BY and HAVING. Every SelectStatement starts a fresh scope, so aliases never cross subquery, CTE or UNION boundaries; qualified references (t.total) always stay columns. Names match case-insensitively, like unquoted identifiers.

Example_analyzeQuery now reports 7 columns instead of 8 (total_spent is an alias).

Implemented with Codex (gpt-6.1-sol) in an isolated worktree; reviewed, gated and benchmarked here.

Test Plan

• New extract_alias_test.go: 19 cases for both extractors (the issue's table, qualified refs, subquery/CTE/UNION scoping both directions, aliases inside ORDER BY expressions, HAVING mixing aliases and real columns), race-clean
• task check: EXIT=0 (fmt, vet, golangci-lint 0 issues, full race suite incl. MCP)
• Benchmarks vs main (precompiled, 12 interleaved rounds): ExtractTables/ExtractColumns ~ (p=0.71 / p=0.98); allocs unchanged except ExtractMetadata 22→24 and MetadataLarge 471→473, i.e. the alias set for queries that actually have aliases + ORDER BY

Breaking Changes

None to the API. Callers of ExtractColumns/ExtractMetadata no longer see aliases listed as columns.

Fixes #849

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
gosqlx-website	Ignored	Preview	Oct 25, 2026 6:00pm UTC

[github-actions]

Claude finished @ajitpratap0's task in 26s —— View job

---

Review of PR #865: SELECT aliases in ORDER BY/HAVING

I read the diff only. I did not run the tests or the linter, and the PR description says task check and the benchmarks were run.

Summary: The fix is correct and well scoped. Scoping is per SelectStatement, matching is case-insensitive, qualified references stay columns, and no alias set is built when there is no ORDER BY/HAVING or no aliases. I found no blocking issues.

What looks good

• Scoping: aliases = nil at the top of the *ast.SelectStatement case gives each query block a fresh scope. Subqueries, CTEs and UNION branches can't inherit outer aliases. The outer alias, inner alias and union branches tests cover this in both directions.
• Alias set is built lazily and only when needed: the guard c.want&(extractColumns|extractColumnsQualified) != 0 && (Having != nil || len(OrderBy) != 0) avoids the map allocation otherwise. That matches the benchmark claim of only +2 allocs for queries that have aliases plus ORDER BY.
• Clause isolation: alias suppression is limited to HAVING and ORDER BY. SELECT, WHERE, GROUP BY and JOIN ON still report the name, and the where/group/select/join unchanged tests lock that in.
• Subqueries inside ORDER BY or HAVING: a nested SelectStatement resets aliases, so ORDER BY (SELECT b FROM u) still reports b (the order subquery and having subquery tests).
• Tests: the table-driven cases are broad and assert both ExtractColumns and ExtractColumnsQualified.

Suggestions (non-blocking)

1. Alias shadowing a real column in HAVING. In SELECT SUM(x) AS x FROM t GROUP BY d HAVING x > 1, x is also a real column. In dialects where HAVING resolves real columns first (Postgres, for example, rejects aliases in HAVING), suppression could hide a genuine reference. The same name is usually still reported from the SELECT-list expression, so the practical impact is small. Consider a one-line comment in extract.go saying this is deliberately a heuristic, plus a test with SELECT SUM(x) AS x ... HAVING x > 1 that pins the intended behavior.
2. Quoted identifiers. Alias names are lowercased unconditionally. A quoted "Total" and a column total would be conflated. This is an edge case, and your doc comment says matching follows unquoted-identifier semantics, so it may be fine to note it as a known limitation.
3. Double traversal in the SELECT case. The explicit clause walks and the trailing node.Children() loop visit the same expressions twice. This is the existing pattern, and the sets are deduplicated, so it is correct. The new comment explains why aliases are re-applied in the second pass, but it is easy to get wrong later. A small helper like aliasesFor(child) might make the intent clearer.
4. Threading nil through every call. Most collectFromExpression(..., nil) call sites exist only to pass nil. Storing the current alias scope on the collector (saved and restored around ORDER BY/HAVING) would shrink the diff and the signature churn. It would be less purely functional, and your current approach is acceptable.
5. CHANGELOG. The entry is fine. It could mention that ExtractMetadata results change, since callers may see that.

The Example_analyzeQuery change from 8 to 7 columns is correct, because total_spent is an alias.

Verdict: Approve once you've considered suggestion 1. The rest is optional.

[github-actions]

Claude finished @ajitpratap0's task in 18s —— View job

---

Review of PR #865: SELECT aliases in ORDER BY/HAVING

I read the diff of pkg/gosqlx/extract.go and the file list. I did not run the tests or linter. The PR description says task check and the benchmarks passed.

Verdict: The fix is correct and well scoped. I found no blocking issues.

What looks good

• Scoping: aliases = nil at the top of the *ast.SelectStatement case gives every query block a fresh scope. Aliases can't leak across subqueries, CTEs or UNION branches.
• Lazy allocation: the alias map is built only when the SELECT has ORDER BY or HAVING, has at least one alias, and the caller wants columns. That matches the +2 allocs you measured for those queries only.
• Clause isolation: suppression applies only to unqualified names in HAVING and ORDER BY. Qualified references (t.total) stay columns.
• Second traversal: the trailing node.Children() loop in the SELECT case now passes aliases only to the OrderBy and Having children. Without that, the generic walk would have re-added the alias names. The new comment explains this.
• Changed return: the SELECT case now ends with return. That avoids a third walk through the generic loop after the switch.

Suggestions (non-blocking)

1. Alias shadowing a real column. SELECT SUM(x) AS x FROM t GROUP BY d HAVING x > 1 suppresses x in HAVING. In dialects that resolve HAVING names to real columns first, that hides a genuine reference. The SELECT-list expression usually still reports x, so the impact is small. A short comment saying this is a deliberate heuristic, plus a test that pins the behavior, would help.
2. Quoted identifiers. strings.ToLower is applied to every alias, so a quoted "Total" and a column total are treated as the same name. This is fine given the doc comment, but worth listing as a known limitation.
3. Signature churn. Most collectFromExpression(..., nil) call sites exist only to pass nil. Keeping the current alias scope on the collector, saved and restored around ORDER BY/HAVING, would shrink the diff. Your explicit-parameter approach is also reasonable and avoids hidden state.
4. Duplicate work. The explicit clause walks and the Children() loop visit ORDER BY and HAVING twice. The results are deduplicated, so this is correct. It is also easy to break later, so a small helper that decides which alias scope applies to a child would make the intent clearer.
5. CHANGELOG. Consider noting that ExtractMetadata and the MCP extract_metadata / analyze_sql outputs change too.

The Example_analyzeQuery change from 8 to 7 columns is correct, because total_spent is an alias.