[ajitpratap0]

Summary

#605 parser/clickhouse: parametric aggregates move DISTINCT to the wrong group and reject chained calls or subscripts

In the ClickHouse fn(params)(args) path, the parser now keeps DISTINCT with the argument group: quantile(0.5)(DISTINCT x) parses and round-trips through SQL() and FormatAST in compact and readable styles. DISTINCT or ORDER BY in the parameter group (uniqExact(DISTINCT x)(y), groupArray(DISTINCT 3)(x)) and a third paren group (f(1)(2)(3)) now fail with E2004 errors that point at the offending token. Before, they were either silently moved into SQL that did not re-parse, or failed with a misleading E2002. A [...] subscript or slice directly after an identifier-named function call (groupArray(x)[1], quantiles(0.5, 0.9)(x)[1], PostgreSQL array_agg(x)[1]) now parses as ArraySubscriptExpression or ArraySliceExpression over the call and round-trips. SQL Server's count(*) [cnt] still renders as AS cnt. The new regression tests fail on 61ca6c8 and pass on this commit, and the full race gate, go vet and golangci-lint are clean. Deferred, with sound reasons:

• Subscripts after keyword-named calls such as IF/REPLACE: the issue's cases use only identifier-named functions.
• Parentheses dropped when rendering (array_agg(x))[1]: a separate render issue.
• ClickHouse fn()(x) rendering as fn(x): pre-existing and not reported in parser/clickhouse: parametric aggregates move DISTINCT to the wrong group and reject chained calls or subscripts #605.
• ORDER BY inside the ClickHouse argument group: not ClickHouse syntax.
• CLI format of ArraySubscriptExpression: belongs to cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599.
• docs/SQL_COMPATIBILITY.md: left for parser/clickhouse: table functions in FROM (numbers(10), remote(...)) fail to parse #606/ast/formatter: SQL() and FormatAST drop table-function call arguments in FROM (generate_series(1,10) -> generate_series) #607 in this chain to avoid conflicts.
  Reviewer note: in SQLite, count(*) [cnt] now parses as a subscript instead of erroring, because SQLite bracket identifiers tokenize as LBRACKET. This matches the existing identifier behaviour but should be gated or followed up.

#606 parser/clickhouse: table functions in FROM (numbers(10), remote(...)) fail to parse

Fixes #606. ClickHouse table functions in FROM (numbers(10), numbers(10) AS n, remote(...), t, numbers(3), file, s3, cluster, generateRandom, plus the keyword-named url, merge, values and null) now parse into a TableReference with TableFunc set, instead of failing with E2002 'expected statement, got LPAREN' or 'expected identifier, got URL'. The fix adds DialectClickHouse to supportsTableFunction(). It also adds a ClickHouse-only branch for keyword-tokenized function names, which applies only when the name is directly followed by '('. Parse behaviour for every other dialect is unchanged; a parent-vs-HEAD probe across mysql, mariadb, sqlite, sqlserver, oracle, generic and postgresql showed no differences. Verified with the regression tests (they fail on the parent at the issue's exact positions and pass on HEAD), the full race gate, go vet and golangci-lint (0 issues). Deferred, with sound reasons: (1) rendering of TableFunc arguments, so numbers(10) still renders as numbers; this is #607, the next commit in this chain, which must add ClickHouse round-trip cases; (2) table functions on the right side of a JOIN (t JOIN numbers(5) AS n), a separate parseJoinedTableRef path that affects other dialects and overlaps the from-joins chain; (3) enabling table functions in the generic dialect; (4) the other pre-existing ClickHouse gaps listed in the issue: view(SELECT ...), url as a column, INSERT INTO FUNCTION, FINAL/SETTINGS dropped on render, and GLOBAL JOIN.

#607 ast/formatter: SQL() and FormatAST drop table-function call arguments in FROM (generate_series(1,10) -> generate_series)

Fixes #607. ast.writeTableRef (used by SQL()) and formatter.tableRefSQL (used by FormatAST) now render the parsed TableReference.TableFunc call. Before, they wrote only TableReference.Name, so generate_series(1,10) AS g came out as generate_series g and the query's meaning changed. This affects PostgreSQL generate_series/unnest/json_each, Snowflake IDENTIFIER/TABLE(...)/FLATTEN, BigQuery UNNEST and ClickHouse numbers/remote/url/merge/values/null/file/cluster. Verified with new regression tests that fail on the parent commit and pass on HEAD, byte-identical round trips through SQL() and the Compact formatter in all four dialects, and the full -race suite, vet and golangci-lint. Deferred:

• The exact Snowflake text FLATTEN(input => t.arr) needs named-argument rendering from ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS #600. Until then it renders FLATTEN(input); the test asserts the full form automatically once => appears.
• The CLI gosqlx format path still drops the arguments until cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599 moves it onto pkg/formatter.

Parser gaps found while probing, to file as follow-ups:

• BigQuery UNNEST([1,2,3]) fails with E2001.
• PostgreSQL table functions on the right side of JOIN / CROSS JOIN LATERAL fail to parse.
• WITH ORDINALITY and column-alias lists after a table function.
• Snowflake outer => TRUE.
• DELETE ... USING / UPDATE ... FROM with a table function.
• The formatter does not render ClickHouse FINAL, TimeTravel or ForSystemTime on table refs.

Commits (4), one implementation commit per issue plus test-only follow-ups:

• 1156be8 fix(parser): keep SQLite [alias] after a function call out of subscript parsing (parser/clickhouse: parametric aggregates move DISTINCT to the wrong group and reject chained calls or subscripts #605)
• 24183bd fix(ast,formatter): render table-function call arguments in FROM (ast/formatter: SQL() and FormatAST drop table-function call arguments in FROM (generate_series(1,10) -> generate_series) #607)
• a63c92b fix(parser/clickhouse): parse table functions in FROM (numbers, remote, file, url, ...) (parser/clickhouse: table functions in FROM (numbers(10), remote(...)) fail to parse #606)
• ec2ad5b fix(parser/clickhouse): keep DISTINCT with the argument group of parametric aggregates and allow subscripts on call results (parser/clickhouse: parametric aggregates move DISTINCT to the wrong group and reject chained calls or subscripts #605)

13 files changed, 747 insertions(+), 26 deletions(-)

Test Plan

• Each issue's reproduction cases were run against main before the fix (Haiku scout), and a failing regression test was written before the code change.
• Independent review (Opus, or Sonnet on small chains) with a bounded fix loop.
• Independent Opus validator ran the repository race gate (CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding)), go vet, golangci-lint, re-ran the reproduction cases on HEAD, and confirmed the new tests fail on the parent commit.
• Perf items: benchstat -count=6 before/after on the same machine.

#605 evidence (validator)

Not a performance item, so no benchstat was run. Repro evidence comes from a scratch module (scratchpad/b3/605/scout) built against HEAD.

clickhouse:
- `quantile(0.5)(DISTINCT x)` parses to FunctionCall params=1 args=1 distinct=true. SQL(), FormatAST compact and FormatAST readable all give quantile(0.5)(DISTINCT x), and each re-parses.
- `uniqExact(DISTINCT x)(y)` fails with E2004 at 1:18 "DISTINCT is not allowed in the parameter list...".
- `groupArray(DISTINCT 3)(x)` fails with E2004 at 1:19.
- `quantiles(0.5,0.9)(x)[1]` parses to ArraySubscriptExpression over FunctionCall and renders as quantiles(0.5, 0.9)(x)[1]. It re-parses.
- `groupArray(x)[1]` parses to ArraySubscriptExpression over FunctionCall and re-parses. [1][2] and [1:2] also work.
- `f(1)(2)(3)` fails with E2004 at 1:15 "takes at most two parenthesized lists".
- Controls `quantile(0.5)(x)` and `topK(3)(x) AS k` are unchanged and round-trip.
- `groupArray(x ORDER BY y)(z)` fails with E2004.
- `quantile(0.9)(x) OVER (...)` round-trips.

Other dialects:
- sqlserver `count(*) [cnt]` still renders as AS cnt.
- postgresql `(array_agg(x))[1]` now renders as array_agg(x)[1], which re-parses.

Parent check (git archive of 61ca6c8 with the new test files copied in): all three new parser tests and TestFormatAST_ClickHouseParametricAggregates FAIL. 13 subtests fail. The only subtests that pass are the single_group_control and sqlserver_bracket_alias_unchanged controls, which are expected to pass before the fix.

#605 race gate: CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) passed with EXIT 0. 44 packages reported ok, with no FAIL and no DATA RACE. Output is in scratchpad/b3/605/race.txt. Targeted run on HEAD: TestClickHouseParametric_DistinctInArgs_605, TestClickHouseParametric_RejectsModifiersInParams_605, TestFunctionCallSubscript_605, TestFormatAST_ClickHouseParametricAggregates and the existing TestClickHouseParametricAggregates, _ASTShape and _Render (#482/#531) all PASS.
#606 evidence (validator)

Not a perf item; evidence is the repro and its fix.

New tests on the parent (ec2ad5b, extracted with git archive, test file copied in): TestClickHouseFromTableFunctions_Issue606 failed in all 14 subtests, and TestClickHouseInsertSelectFromTableFunction_Issue606 also failed. On HEAD all of them pass.

Scout probes through gosqlx.ParseWithDialect, parent vs HEAD:
- Parent errors match the issue exactly: numbers(10) E2002 1:22; numbers(10) AS n 1:27; remote(...) 1:21; `t, numbers(3)` 1:25; file(...) 1:19; url(...) 'expected identifier, got URL' 1:15.
- On HEAD each parses with TableFunc set:
  - numbers: 1 arg
  - numbers AS n: alias n
  - remote: 3 args
  - `t, numbers(3)`: From[1].TableFunc set, From[0] is the plain table t
  - file: 2 args
  - url: 3 args
- The parent-vs-HEAD diff changes only ClickHouse table-function inputs. That includes extra probes `numbers(10) FINAL` (Final=true is kept), `numbers(10) n WHERE ...` and `LATERAL numbers(3)`.
- Unchanged:
  - `numbers(10)` in mysql, sqlite, sqlserver, oracle and mariadb (E2002 1:22).
  - generic `generate_series(1,10) g` (E2002 1:30).
  - mysql `url('x')` and postgresql `values('x')` (still 'expected identifier').
  - ClickHouse `FROM url`, `FROM merge` and `FROM values` without '(' (still error).
  - ClickHouse `t FINAL`, `t SAMPLE 0.1`, `(SELECT 1)`, `db.t AS x`, `ARRAY JOIN`, `INSERT ... VALUES`.
  - ClickHouse `t JOIN numbers(5) AS n` (still errors; out of scope).
- PostgreSQL generate_series still renders without its arguments ('FROM generate_series g'). That is #607 and is expected.

CLI on HEAD: `gosqlx validate --dialect clickhouse "SELECT * FROM numbers(10)"` reports ✅ Valid SQL. The mysql, generic and sqlite dialects still give ❌ E2002 1:22.

Probe outputs: scratchpad/b3/606/scout_parent.txt and scout_head.txt

#606 race gate: PASS. Ran CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) on HEAD a63c92b and it exited 0. All 44 test packages reported ok and none failed. pkg/sql/parser (9.27s), pkg/gosqlx (3.25s) and pkg/formatter (1.94s) ran fresh, not from cache. Log: /private/tmp/claude-501/-Users-ajitpratapsingh-dev-GoSQLX/b99cd3cf-cb92-4294-a0db-d6a7c9a86253/scratchpad/b3/606/race.txt
#607 evidence (validator)

This is a render bug fix, not a perf item, so no benchstat was run. The repro and round-trip evidence below comes from an independent probe module in scratchpad/b3/607/vprobe, pointed at the worktree with a replace directive. On HEAD:
- postgresql `SELECT * FROM generate_series(1,10) AS g`: SQL() is `SELECT * FROM generate_series(1, 10) g` and gosqlx.FormatAST is "SELECT *\nFROM generate_series(1, 10) g". Was `generate_series g`.
- postgresql `SELECT * FROM generate_series(1,10)` gives `SELECT * FROM generate_series(1, 10)`.
- postgresql unnest(ARRAY[1,2,3]) u and json_each(...) j keep their arguments.
- bigquery `UNNEST(t.arr) x` and `UNNEST(ARRAY[1, 2, 3]) x` keep their arguments.
- snowflake `IDENTIFIER('t1')` keeps its argument.
- clickhouse numbers, remote, url, merge, values, null, file and cluster keep their arguments in SQL(), Compact, Readable and gosqlx.FormatAST.
- Every case round-trips. parse -> SQL() -> parse -> SQL() is byte-identical, and parse -> Compact -> parse -> SQL() is identical as well.
- Controls are unchanged: plain JOIN, derived table, LATERAL subquery and Snowflake @stage/path.
- snowflake `LATERAL FLATTEN(input => t.arr) f` now renders `LATERAL FLATTEN(input) f`. The call and its parentheses are back, but the named-arg value is still missing until #600 lands. Same for TABLE(FLATTEN(input)).
- bigquery `UNNEST([1,2,3])` still fails with E2001 at LBRACKET. This is a pre-existing parser gap and out of scope.

Fail-before check: I exported the parent 24183bd^ with git archive into scratchpad/b3/607/parent, copied in the two new test files and ran them. They fail there: TestTableFunctionSQL_Issue607, TestTableFunctionSQL_NamedArgs_Issue607, TestFormatAST_TableFunctions_Issue607, TestFormatAST_TableFunctions_PublicAPI_Issue607 and TestFormatAST_TableFunctions_NamedArgs_Issue607 all FAIL. All five PASS on HEAD under -race.

#607 race gate: PASS. Ran CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) on HEAD 24183bd and it exited 0. 44 packages reported ok with no FAIL lines, including pkg/formatter (4.6s), pkg/gosqlx (8.6s), pkg/sql/ast (6.5s) and pkg/sql/parser (19.1s). There were no flakes, so nothing was re-run. Log: /private/tmp/claude-501/-Users-ajitpratapsingh-dev-GoSQLX/b99cd3cf-cb92-4294-a0db-d6a7c9a86253/scratchpad/b3/607/race.txt

Breaking Changes

• parser/clickhouse: parametric aggregates move DISTINCT to the wrong group and reject chained calls or subscripts #605: None to exported identifiers or AST types. The observable behaviour changes are all additive or error-clarifying.
• clickhouse fn(p)(DISTINCT a) now parses, with FunctionCall.Distinct=true meaning DISTINCT on the args.
• clickhouse fn(DISTINCT p)(a) and fn(p ORDER BY o)(a) now return E2004. Previously they parsed and were silently rewritten into SQL that does not re-parse.
• clickhouse fn(a)(b)(c) now returns E2004 instead of E2002.
• In every dialect, a [...] subscript or slice directly after an identifier-named function call now parses as ArraySubscriptExpression/ArraySliceExpression wrapping the FunctionCall. Previously this was an E2002 error.
• parser/clickhouse: table functions in FROM (numbers(10), remote(...)) fail to parse #606: None. No exported identifier, AST field or signature changes. The only observable change is additive: in the ClickHouse dialect, function-call table references in the FROM list (including the keyword-named url/merge/values/null) now parse into the existing TableReference.TableFunc field instead of failing with E2002. All other dialects behave as before.
• ast/formatter: SQL() and FormatAST drop table-function call arguments in FROM (generate_series(1,10) -> generate_series) #607: No exported API or signature change and no new AST fields; pool release and Children() are unchanged, since TableFunc was already released and traversed. Observable behaviour: TableReference.SQL() (through SelectStatement/AST.SQL()), formatter.FormatAST/FormatStatement and gosqlx.FormatAST now emit the full table-function call (name plus argument list, through FunctionCall.SQL()) instead of only the name when TableFunc is set. This is the bug fix; output for inputs without a table function is byte-identical.

Notes

• CHANGELOG.md is intentionally not touched; the consolidated CHANGELOG PR docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 gets the entries for this batch before it merges.
• Part of the batch-3 merge train (follow-ups cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599-schema/postgres: loader misreports index columns and key flags (column order, INCLUDE, expression and partitioned indexes, read-only roles) #620); later PRs in the train are rebased onto this one after it merges.
• Base: main.

Fixes #605
Fixes #606
Fixes #607
Follow-up to #566

🤖 Generated with Claude Code

https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve

[vercel]

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

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 14, 2026 6:51am UTC

[github-actions]

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

---

Review of PR #628 (ClickHouse parametric aggregates, FROM table functions, rendering)

I read the non-test source diff. I did not build the project or run the tests. I did not read the new test files in full.

Overall: The changes are small and well scoped. The error messages are clear. Each fix is gated tightly to the dialect it targets, and the PR description is unusually thorough. I found no blocking bugs.

What looks good

• parseClickHouseParametricArgs (clickhouse_parametric.go) pulls the logic out of parseFunctionCall. It also rejects DISTINCT or ORDER BY in the parameter group, and a third paren group, with E2004 at the offending token.
• isClickHouseKeywordTableFunction requires the next token to be ( and the dialect to be ClickHouse. A plain FROM url or FROM values therefore keeps its old behaviour.
• TableFunc rendering is added in both ast.writeTableRef and formatter.tableRefSQL. The Subquery branch is checked first, so there is no ordering risk.

Points to consider

1. Subscript parsing is not gated for SQL Server (expressions_literal.go). Only SQLite is excluded. SQL Server also uses [alias], as in count(*) [cnt]. The PR description says it still renders as AS cnt, which suggests the tokenizer yields an identifier there. A parser test that locks in this behaviour for SQL Server would help. For MySQL and PostgreSQL, count(*) [x] now parses as a subscript, where before it was an error.
2. SQLite loses array indexing on function calls. This is the trade-off the PR already flags. It is acceptable, but a follow-up issue would be good so it isn't forgotten.
3. Keyword-named table functions skip the generic path. The else if branch in select_subquery.go builds a TableReference without schema qualification. That is fine for url(...). A qualified form such as db.url(...) still goes through the old path. This is an edge case and not worth blocking.
4. ORDER BY check position. In parseClickHouseParametricArgs, ORDER BY is reported at p.currentLocation(), which is the second (. DISTINCT is reported at its own token. The caret for ORDER BY therefore points at the paren and not at ORDER. Capturing the ORDER BY position, as is done for DISTINCT, would make the errors consistent.
5. Docs. docs/SQL_COMPATIBILITY.md gains only the table-function line. The quantile(0.5)(DISTINCT x) form and the subscript-on-call behaviour could be mentioned too. The PR says CHANGELOG is deferred to docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598, which is fine.
6. Known gaps that are already deferred in the PR: fn()(x) renders as fn(x), (array_agg(x))[1] loses its parentheses on render, and JOIN numbers(5) fails to parse. The deferrals look reasonable, but they make the round-trip guarantees partial for those cases.

Performance and security

• There are no new allocations on hot paths. The extra check is a single token-type comparison.
• I see no security concerns. The new code only adds parser branches that return errors on malformed input.

Tests

The PR description reports regression tests that fail on the parent commit and pass on HEAD, plus a clean race gate. Coverage of the new paths looks proportionate. I would add one parser test for count(*) [cnt] under MySQL or PostgreSQL, to document the new subscript-versus-error behaviour.

Verdict: Approve with minor, non-blocking suggestions (points 1 and 4).
· branch fix/clickhouse-aggregates-table-functions

[github-actions]

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

---

Review of PR #628 (ClickHouse parametric aggregates, FROM table functions, rendering)

I read the non-test source diff (parser, ast, formatter, security scanner, transform). I did not build the project or run the tests. I did not read the new test files in full.

Overall: The changes are well scoped and the dialect gating is tight. I found no blocking bugs. The earlier automated review did not cover the security and transform changes, so I've added them below.

What looks good

• parseClickHouseParametricArgs (clickhouse_parametric.go) moves the logic out of parseFunctionCall. It rejects DISTINCT or ORDER BY in the parameter group, and a third paren group, with E2004. Distinct now applies to the argument group.
• isClickHouseKeywordTableFunction requires both ClickHouse and a directly following (, so a plain FROM url or FROM values is unchanged.
• renameTableRef in pkg/transform/tables.go renames TableFunc.Name along with Name. Without that, the new rendering would have emitted the old name. This is a good catch.
• scanTableReference scans Joins[i].Right only. It avoids double-scanning Left, and the comment explains why.
• The scanner now descends into ArraySubscriptExpression and ArraySliceExpression. That closes a bypass such as pg_sleep(5)[1], which the new subscript parsing would otherwise have opened.

Points to consider

1. Subscript after a call is gated only for SQLite (expressions_literal.go:~184). The PR says SQL Server count(*) [cnt] still renders as AS cnt. I couldn't confirm that in this run, so a parser test pinning the SQL Server behaviour would be worth having. In MySQL and PostgreSQL, count(*) [x] now parses as a subscript where it used to be an error. A test documenting that would help. The SQLite loss of f(x)[1] should get a follow-up issue so it isn't forgotten.
2. ORDER BY error position. In parseClickHouseParametricArgs the ORDER BY error uses p.currentLocation(), which is the second (. The DISTINCT error uses its own token position. Capturing the ORDER BY position, as is done for DISTINCT, would make the carets consistent.
3. Scanner: possible duplicate findings. I added FROM|JOIN <fn>( to the regex pattern list in scanner.go. scanTableReference also emits an out-of-band finding from the AST for the same query. If both run on a query, the user may get two Critical findings for one FROM url(...). Please check whether findings are de-duplicated, or add a test that asserts the count.
4. Scanner: dialect scope. outOfBandTableFuncNames is applied regardless of dialect, and the regex is also dialect-agnostic. For example, FROM mysql( or FROM file( in a dialect where that is a user function would be flagged Critical. The comment says "ClickHouse", so either restrict it or note that it is deliberately broad. This is a security-conservative default, so it is not blocking.
5. Qualified keyword-named table functions. db.url(...) still goes through the old path (select_subquery.go). This is a rare edge case.
6. Docs. docs/SQL_COMPATIBILITY.md gets one line. The quantile(0.5)(DISTINCT x) form and subscripts on call results could be mentioned too. Deferring the CHANGELOG to docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 is fine.
7. Known deferred gaps (already listed in the PR): fn()(x) renders as fn(x), (array_agg(x))[1] loses its parentheses on render, and JOIN numbers(5) doesn't parse. Round-trips are partial for those cases.

Performance and security

• There are no new allocations on hot paths. The new parser checks are single token-type comparisons.
• Security is net positive: the scanner now covers table functions and subscripted calls. Point 3 is the main thing to verify.

Tests

The reported regression tests fail on the parent commit and pass on HEAD, and the race gate is clean. I'd add the following:

• a SQL Server count(*) [cnt] parser test;
• a MySQL or PostgreSQL count(*) [x] test;
• a scanner test that asserts the finding count for FROM url(...).

Verdict: Approve with minor, non-blocking suggestions (points 1 and 3).
· branch fix/clickhouse-aggregates-table-functions

[ajitpratap0]

Merge train (batch 3): rebased onto main; head 200644c.

• Adversarial pass (Opus): found no must-fix items. Two should-fix items were fixed in c4828a7 and validated by Opus:
  • transform.ReplaceTable no longer silently fails to rename table-function references.
  • The AST security scanner now descends into table-function arguments, so url(...), file(...) and similar are detected.
• SQLite count(*) [cnt]: the alias-loss regression was fixed earlier (1156be8).
• Bot point 1: added TestFunctionCallSubscript_605_SQLServerBracketAlias, which pins that SQL Server keeps [cnt] as an alias. In MySQL and PostgreSQL, count(*) [x] now parses as a subscript. The legacy CLI renders any subscript as [], a gap that predates this PR (arr[1] does the same on main). fix(cli): render gosqlx format through pkg/formatter and refuse non-round-tripping output (#599) #624 later in this train moves the CLI to pkg/formatter.
• Bot point 4 (ORDER BY caret position): cosmetic, noted for follow-up.
• Follow-ups: JOIN-side table functions, fn()(x), and scanner coverage of derived tables and CTEs go to follow-up issues.

[github-actions]

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

---

Review of PR #628 (ClickHouse parametric aggregates, FROM table functions, rendering)

I read the source diffs for the parser, ast, formatter, security and transform changes, and the full diffs of clickhouse_parametric.go, expressions_literal.go and scanner.go. I did not build the project or run the tests, and I did not read the test files in full.

Overall: The changes are well scoped, dialect gating is tight, and I found no blocking bugs. Two points are worth a look before merge: a possible duplicate scanner finding (#1) and a false-positive risk in the scanner regex (#2).

Good

• parseClickHouseParametricArgs (pkg/sql/parser/clickhouse_parametric.go) now has Distinct meaning DISTINCT on the argument group. Misplaced DISTINCT or ORDER BY, and a third paren group, give clear E2004 errors.
• isClickHouseKeywordTableFunction requires both ClickHouse and a directly following (, so a plain FROM url is unchanged.
• renameTableRef in pkg/transform/tables.go renames TableFunc.Name too. Without it, the new rendering would have emitted the old name.
• The scanner now descends into subscript and slice expressions. That closes a bypass such as pg_sleep(5)[1], which the new subscript parsing would otherwise have opened.
• scanSelectStatement scans only Joins[i].Right, which avoids double-counting Left.

Points to consider

1. Possible duplicate scanner findings (pkg/sql/security/scanner.go). The new regex FROM|JOIN <fn>( and scanTableReference can both fire on FROM url(...). That would give two Critical findings for one construct. Please confirm the two paths don't both report, or add a test that asserts exactly one finding.
2. Scanner dialect scope. The regex and outOfBandTableFuncNames are dialect-agnostic. FROM mysql(, FROM file( and FROM sqlite( are flagged Critical in every dialect. This is conservative and acceptable. The comments should say it is deliberate, since they currently describe it as ClickHouse-only. The regex is also text-based, so a string literal containing FROM url( would trigger it. That is typical of the existing patterns.
3. Subscript on a call result is gated only for SQLite (pkg/sql/parser/expressions_literal.go, around line 184).
   • In MySQL, PostgreSQL and generic, count(*) [x] used to error and now parses as a subscript. A test documenting that would help.
   • The PR description says SQL Server count(*) [cnt] still renders as AS cnt, and a locking test was added in the latest commit, so that case looks covered.
   • Losing f(x)[1] in SQLite should get a follow-up issue.
4. ORDER BY error position. In parseClickHouseParametricArgs the ORDER BY error uses p.currentLocation(), which is the second (. The DISTINCT error points at its own token. Capturing the ORDER BY position the same way would make the carets consistent.
5. Qualified keyword-named table functions. db.url(...) still takes the old path. This is a rare edge case and not worth blocking.
6. Docs. docs/SQL_COMPATIBILITY.md gets one line. quantile(0.5)(DISTINCT x) and subscripts on call results could also be mentioned. Deferring the CHANGELOG to docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 is fine.
7. Known deferred gaps (already listed in the PR): fn()(x) renders as fn(x), (array_agg(x))[1] loses its parentheses, and JOIN numbers(5) doesn't parse. Round-trips are partial for those.

Performance and security

• No new allocations on hot paths. The new parser checks are single token-type comparisons.
• Security is net positive. The only concern is the duplicate findings in Welcome to GoSQLX Discussions! #1.

Tests

The PR reports regression tests that fail on the parent and pass on HEAD, plus a clean race gate. I'd add:

• a scanner test asserting the finding count for FROM url('http://x') (Welcome to GoSQLX Discussions! #1);
• a MySQL or PostgreSQL count(*) [x] test (go.mod has incorrect module name #3).

Verdict: Approve with minor, non-blocking suggestions (#1, #3, #4).
· branch fix/clickhouse-aggregates-table-functions