Skip to content

fix(parser,tokenizer): ClickHouse/Snowflake lambda bodies and U& in dialects without & [W0-00a] - #901

Merged
ajitpratap0 merged 5 commits into
mainfrom
perf/w0-00a-transpile-correctness
Oct 31, 2026
Merged

ajitpratap0 merged 5 commits into
mainfrom
perf/w0-00a-transpile-correctness

Conversation

@ajitpratap0

@ajitpratap0 ajitpratap0 commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner

Summary

This PR fixes the two transpile correctness bugs from perf-audit CRITIQUE item 14 (dive-transpile-bindings §4). They had to be fixed before the W0-02 transpile goldens are captured, so the goldens do not record the wrong output as expected.

Fixes #897
Fixes #898

#897: ClickHouse and Snowflake lambdas. The parser read -> as the JSON arrow, which binds tighter than every other operator. arrayMap(x -> x * 2, a) therefore became (x -> x) * 2 in the AST. Formatting for ClickHouse or Snowflake then emitted arrayMap((x -> x) * 2, a). This happened in identity transpiles, in ClickHouse to Snowflake transpiles and in FormatASTChecked.

  • Parser (parseJSONExpression): in ClickHouse and Snowflake, -> now starts a lambda whose body is a full parseExpression(). Neither dialect has a JSON ->. ->> is not an expression operator in either dialect (Snowflake's ->> is the statement-level flow operator), so parsing it there now returns an E4001 error, the ->> operator is not supported in <dialect>. On main it parsed as a JSON node and formatted unchanged; without this parse-time check the JSON-arrow refusal below would have failed same-dialect formatting and transpiling of such input with an error naming the source dialect as the target.
  • AST: a new additive field, BinaryExpression.Lambda, marks these nodes. It follows the BitwiseXor, XorAtAdditive and PipesOr pattern.
    • expressionPrecedence (used by SQL() on hand-built trees) gives a lambda the lowest precedence.
    • targetExpressionPrecedence (used by RetargetOperators in the formatter and transpiler) does the same for the ClickHouse and Snowflake targets.
  • Retarget (binaryTargetForm): a lambda sent to a dialect without lambdas (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server, Oracle) is refused with the unsupported-operator retarget error, the lambda operator -> has no equivalent in <target>. Written as that dialect's ->, it would mean JSON extraction (MySQL: arrayMap(x -> (x * 2), arr)) or regroup as (x -> x) * 2 (SQL Server, Oracle). The generic dialect keeps the lambda but, like the JSON ->, groups its body: arrayMap(x -> x * 2, arr) from ClickHouse or Snowflake to generic is now arrayMap(x -> (x * 2), arr) (it was arrayMap(x -> x * 2, arr), which the generic parser reads as (x -> x) * 2).
  • Reverse direction: a JSON -> or ->> (Lambda == false) sent to ClickHouse or Snowflake is refused with the JSON operator -> has no equivalent in <target>. There -> is the lambda operator and ->> is Snowflake's flow operator. Before this, GoSQLX emitted data -> 'a' = 'x' unchanged, and its own ClickHouse/Snowflake parser now reads that back as the lambda data -> ('a' = 'x').

#898: U&'...' in Oracle, Snowflake and ClickHouse. None of these dialects has U& literals or an & operator. The tokenizer still split U&'...' into U & '...', and formatting emitted BITAND(U, '...') or bitAnd(U, '...').

  • Tokenizer (readIdentifier): an adjacent U&' or U&" in these three dialects now returns an E4001 unsupported-feature error at the U&.
  • Unchanged:
    • PostgreSQL still lexes U& literals.
    • MySQL, MariaDB, SQL Server and SQLite still read U&'x' as U & 'x', because they have &.
    • A spaced U & '...' is unchanged everywhere.

The CHANGELOG has an [Unreleased] ### Fixed entry, as the task requires.

Affected paths (before this PR)

input path before after
arrayMap(x -> x * 2, [1, 2, 3]) clickhouse/snowflake -> clickhouse/snowflake, FormatASTChecked arrayMap((x -> x) * 2, ...) unchanged input
same clickhouse/snowflake -> postgresql, sqlserver, oracle (x -> x) * 2 error: no lambda in target
same clickhouse/snowflake -> mysql, mariadb, sqlite x -> x * 2 (re-parses as (x -> x) * 2) error: no lambda in target
arrayMap(x -> x * 2, ...) clickhouse/snowflake -> generic x -> x * 2 (re-parses as (x -> x) * 2) x -> (x * 2)
SELECT data -> 'a' = 'x', data ->> 'a' postgresql/mysql/mariadb/sqlite -> clickhouse/snowflake unchanged (re-read as a lambda after this PR's parser fix) error: no JSON arrow in target
SELECT a ->> 'b' FROM t clickhouse/snowflake parse, identity transpile, FormatWithDialect SELECT a ->> 'b' FROM t E4001 parse error: ->> not supported
SELECT U&'d\0061t' oracle/snowflake identity BITAND(U, '...') E4001 error
same clickhouse identity bitAnd(U, '...') E4001 error

Plain gosqlx.Format (which parses and prints with the generic dialect, where -> is the JSON arrow) is unchanged. The only generic-dialect output change is the ClickHouse/Snowflake -> generic transpile row above. Hot paths: the tokenizer change only adds a switch inside the existing branch for the rare U&' case. The parser check runs only inside the non-PostgreSQL JSON-operator loop. So no benchmark was run.

Test Plan

New behaviour tests. Each one fails on main and passes here; I checked this by reverting only the four source files:

  • pkg/sql/parser/lambda_test.go:
    • The lambda body is the whole expression (*, -, AND, OR, LIKE, IN, tuple parameters, nested lambdas) in ClickHouse and Snowflake.
    • JSON -> in MySQL, MariaDB, SQLite and PostgreSQL is not a lambda.
    • FormatASTChecked round-trips across ClickHouse and Snowflake.
  • pkg/sql/ast/lambda_precedence_test.go:
    • SQL() of a hand-built lambda does not group its body.
    • RetargetOperators keeps the lambda for ClickHouse, Snowflake and the generic dialect, and refuses it for all 6 dialects without lambdas.
    • RetargetOperators refuses JSON ->/->> for ClickHouse and Snowflake and keeps them for the JSON dialects.
  • pkg/transpiler/lambda_unicode_test.go:
    • 11 lambda statements are unchanged across the four ClickHouse/Snowflake pairs.
    • TestTranspileLambdaToGeneric: the same 11 statements plus arrayMap(x -> x * 2, arr) FROM t, from ClickHouse and Snowflake to DialectGeneric, have pinned outputs with the body parenthesized (x -> (x * 2)).
    • All 11 lambda statements from ClickHouse and Snowflake to PostgreSQL, MySQL, MariaDB, SQLite, SQL Server and Oracle return the lambda refusal.
    • JSON ->/->> statements from PostgreSQL, MySQL, MariaDB and SQLite to ClickHouse and Snowflake return the JSON-operator refusal. PostgreSQL to MySQL keeps ->.
    • U& is rejected from Oracle, Snowflake and ClickHouse to all 9 targets.
    • Pinned outputs for the unaffected dialects.
  • TestLongArrowRejectedForLambdaDialects (parser) and the same-dialect case in TestTranspileJSONArrowRefusedForLambdaDialects: ->> in ClickHouse and Snowflake is an E4001 parse error, including for clickhouse->clickhouse and snowflake->snowflake transpiles; MySQL, MariaDB, SQLite and PostgreSQL still parse it. Both fail with the parser check reverted.
  • pkg/sql/tokenizer/unicode_escape_test.go: the E4001 code and the location of the U& rejection. Spaced U & 'x', menu&'x' and U&x are still accepted.
  • TestPrefilterEquivalence: four inputs added, as CRITIQUE item 14 asks.

Updated expectations:

  • TestOtherDialectsKeepUnicodeAmpersandTokens listed Oracle as a dialect that keeps the three U & 'x' tokens. Oracle moves to the new rejection test, and MariaDB is added to the list.
  • tokenizer_literal_golden.json: I regenerated it and kept only the changed entries. A structural check confirms that only 111 digests changed, all for oracle, snowflake or clickhouse and all on U& inputs.

Gate (AC-STD), run in the worktree:

  • task check (fmt:check, vet, golangci-lint, test:race): pass
  • task test:cbinding: pass
  • go test -count=1 ./pkg/compatibility/...: pass
  • go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/ast/ ./pkg/sql/parser/ ./pkg/sql/tokenizer/ ./pkg/transpiler/: no findings
  • Review fix (72bb748), all re-run in the worktree: task check EXIT 0; task test:cbinding ok; go test -count=1 ./pkg/compatibility/... ok; staticcheck@latest on ./pkg/sql/ast/ ./pkg/transpiler/ clean. The pre-commit hook's short run failed once on the timing test TestFormatASTExpressionLinearGrowth (load average 33). It passed on re-run, and in task check.
  • Second review fix (454c14c, ->> parse rejection), re-run in the worktree: task check EXIT 0; task test:cbinding EXIT 0; go test -count=1 ./pkg/compatibility/... EXIT 0; go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/parser/... ./pkg/sql/ast/... ./pkg/sql/tokenizer/... ./pkg/transpiler/... EXIT 0, no findings.
  • Third review fix (8c63594, generic-target disclosure and TestTranspileLambdaToGeneric; test and CHANGELOG only), re-run in the worktree: task check EXIT 0; task test:cbinding EXIT 0; go test -count=1 ./pkg/compatibility/... EXIT 0; go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/transpiler/... EXIT 0, no findings.
  • go test -count=1 -run TestPerformanceRegression ./pkg/sql/parser/: failed once in a non-race full run while the shared machine had a load average of about 11. It passed on re-run, about 50% faster than baseline on every case.

Breaking Changes

  • No exported signature changes. There is one additive field, ast.BinaryExpression.Lambda. JSON dumps of the AST gain "Lambda": false|true.
  • Behaviour:
    • In ClickHouse and Snowflake, -> now parses as a lambda, so the AST shape changes: Right is the whole body.
    • Transpiling a ClickHouse/Snowflake lambda to PostgreSQL, MySQL, MariaDB, SQLite, SQL Server or Oracle now returns an error. It used to emit a -> that meant something else in the target.
    • Transpiling a ClickHouse/Snowflake lambda to the generic dialect now parenthesizes its body: arrayMap(x -> x * 2, arr) becomes arrayMap(x -> (x * 2), arr) (on main: arrayMap(x -> x * 2, arr)).
    • Transpiling a JSON ->/->> to ClickHouse or Snowflake now returns an error. It used to be emitted unchanged.
    • ->> with the ClickHouse or Snowflake dialect (parse, validate, FormatWithDialect, and same-dialect transpile) now fails with an E4001 parse error, the ->> operator is not supported in <dialect>. On main, SELECT a ->> 'b' FROM t parsed in these dialects and round-tripped unchanged; neither dialect has ->> as an expression operator.
    • U&'...' and U&"..." in Oracle, Snowflake and ClickHouse now fail to tokenize with E4001, where they used to be accepted as a bitwise AND.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

…t U& where & does not exist

- In ClickHouse and Snowflake, -> is now parsed as the lambda operator.
  Its body is a whole expression, so arrayMap(x -> x * 2, a) is no longer
  (x -> x) * 2. The new BinaryExpression.Lambda field marks these nodes,
  and the SQL() and retarget precedence tables treat a lambda as looser
  than OR in lambda dialects. Identity and ClickHouse<->Snowflake
  transpiles now keep the lambda unchanged. Transpiles to JSON-arrow
  dialects keep the body grouped as the target parses it. (#897)
- Oracle, Snowflake and ClickHouse have neither U& literals nor an &
  operator. In these dialects U&'...' and U&"..." now return an E4001
  tokenizer error. They used to be read as U & '...' and formatted as
  BITAND(U, '...') or bitAnd(U, '...'). (#898)

Fixes #897
Fixes #898

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
@vercel

vercel Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
gosqlx-website Ignored Ignored Preview Oct 31, 2026 8:10pm UTC

@github-actions github-actions Bot added documentation Improvements or additions to documentation parser tokenizer Tokenizer / lexer changes ast AST node changes labels Oct 31, 2026
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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


Review of PR #901: ClickHouse/Snowflake lambdas and U& in dialects without &

I read the source diff (AST, parser, tokenizer). I did not run the test suite. The PR description reports task check and cbinding passing.

Verdict: The change is small, well scoped and well tested. I found no blocking issues.

What looks good

  • Additive AST field. BinaryExpression.Lambda follows the existing BitwiseXor, XorAtAdditive and PipesOr pattern. There are no signature changes.
  • Parser (pkg/sql/parser/expressions.go, parseJSONExpression):
    • The lambda check runs only inside the non-PostgreSQL JSON-operator loop, so it adds nothing to hot paths for other dialects.
    • Using parseExpression() for the body gives the right "lowest precedence" semantics.
  • Precedence handling is consistent in both places. expressionPrecedence and targetExpressionPrecedence both return 0 for a lambda. For targets with a JSON ->, the body is grouped as x -> (x * 2), which re-parses correctly in MySQL, MariaDB and SQLite.
  • Tokenizer:
    • The switch is only entered on the rare U&' or U&" path.
    • The E4001 error carries context and a hint.
    • PostgreSQL behaviour and the &-capable dialects are unchanged.
  • Tests:
    • The lambda tests cover re-parsing in the target dialect, round trips and JSON -> in the other dialects.
    • The negative cases (menu&'x', U&x, spaced U & 'x') are included.
    • The golden JSON change is limited to the oracle, snowflake and clickhouse U& inputs.

Minor observations (non-blocking)

  1. Lambda placement in parseJSONExpression. It returns the lambda as soon as it sees ->, whatever left is. This is fine for the normal forms (x -> ..., (x, y) -> ...). Because this level binds tighter than * and +, an input like 2 * x -> x + 1 parses as 2 * (x -> x + 1). That input is invalid in both dialects anyway, so this is only about the quality of the error. You could optionally check that left is an identifier or a tuple of identifiers and otherwise return a parse error. That would reject nonsense like a.b -> c or f(x) -> c earlier, but I don't think it's needed here.
  2. Lambda outside the first loop iteration. After a lambda is returned, the loop never sees a following JSON operator. Because the body consumes everything, that is correct. A one-line comment saying so would help the next reader.
  3. Dialect check duplication. arrowIsLambda() (parser) and lambdaTarget() (ast) encode the same ClickHouse/Snowflake rule independently. The comment links them, but a future dialect addition has to touch both. This is acceptable given the package boundary.
  4. Breaking behaviour. Oracle, Snowflake and ClickHouse now hard-fail on U&'...'. This matches the described intent and is documented in the CHANGELOG. It may be worth a one-line mention in docs/SQL_COMPATIBILITY.md.
  5. JSON dumps of the AST gain "Lambda". The PR notes this, so just be aware if any golden AST JSON exists outside the files touched here.

Overall this looks good to merge once CI is green.

…rrows for ClickHouse/Snowflake

- A lambda (BinaryExpression.Lambda) transpiled to PostgreSQL, MySQL,
  MariaDB, SQLite, SQL Server or Oracle is now refused with the
  unsupported-operator retarget error. Written as that dialect's -> it
  meant JSON extraction (MySQL) or regrouped as (x -> x) * 2 (SQL Server,
  Oracle). The generic dialect is unchanged.
- A JSON -> or ->> transpiled to ClickHouse or Snowflake is now refused:
  there -> is the lambda operator (and ->> Snowflake's flow operator), so
  GoSQLX re-read data -> 'a' = 'x' as the lambda data -> ('a' = 'x').
- Tests now assert the refusal for all six non-lambda targets and for
  PostgreSQL/MySQL/MariaDB/SQLite -> ClickHouse/Snowflake; the CHANGELOG
  entry describes the refusals instead of the grouped -> output. (#897)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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


Review of PR #901: ClickHouse/Snowflake lambdas and U& in dialects without &

I read the source diff (AST, parser, tokenizer, retargeting). I did not run the tests. The PR description reports task check and cbinding passing.

Verdict: I found no blocking issues. The change is well scoped and well tested.

What looks good

  • Additive AST field. BinaryExpression.Lambda follows the BitwiseXor, XorAtAdditive and PipesOr pattern, with no signature changes.
  • Parser. parseLambdaBody uses parseExpression() for the body, so the lambda binds looser than everything else. The check sits inside the non-PostgreSQL JSON loop, so it adds no cost to other paths.
  • Retargeting. The lambda is threaded through binaryTargetForm, nestsOutput and targetExpressionPrecedence.
    • Lambdas are refused in dialects without them.
    • JSON -> and ->> are refused in ClickHouse and Snowflake.
    • Other targets get x -> (x * 2), which re-parses correctly.
  • Tokenizer. The new switch is only entered on the rare U&' or U&" path. The E4001 error carries context and a hint. PostgreSQL and the dialects that have & are unchanged.
  • Tests. The round-trips, re-parsing in the target dialect and the negative cases (menu&'x', U&x, spaced U & 'x') give good coverage. The golden JSON change is limited to the oracle, snowflake and clickhouse U& inputs.

Minor observations (non-blocking)

  1. No validation of lambda parameters. parseJSONExpression returns a lambda for any left. So 2 * x -> x + 1 parses as 2 * (x -> x + 1), and a.b -> c or f(x) -> c parse as lambdas. Both dialects reject these, so this only affects error quality. Checking that left is an identifier or a tuple of identifiers would reject them earlier.
  2. Explain the early return. A one-line comment in the loop saying that the lambda body consumes everything, so no further JSON operator can follow, would help future readers.
  3. Duplicated dialect rule. arrowIsLambda() (parser) and lambdaTarget() (ast) encode the same ClickHouse/Snowflake rule. The comment cross-references them, which is fine given the package boundary. A shared test that asserts they agree would catch drift.
  4. Docs. Oracle, Snowflake and ClickHouse now hard-fail on U&'...'. The CHANGELOG covers this. A line in docs/SQL_COMPATIBILITY.md would also help.
  5. AST JSON dumps gain "Lambda". Check for any golden AST JSON outside the files touched here.

Overall this looks good to merge once CI is green.

ClickHouse and Snowflake still built a JSON ->> node, which the lambda-
dialect JSON-arrow refusal then rejected when formatting or transpiling,
even within the same dialect, with an error naming the source dialect as
the target. Neither dialect has ->> as an expression operator (Snowflake's
->> is the statement-level flow operator), so parsing it there now returns
an E4001 unsupported-feature error. Tests pin the parse error and the
same-dialect transpile; the CHANGELOG #897 entry records the change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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


Review of PR #901: ClickHouse/Snowflake lambdas and U& in dialects without &

I read the source diff for the parser, tokenizer, AST and retargeting. I did not run the tests. The PR description reports task check, cbinding and compatibility passing.

Verdict: I found no blocking issues. The change is well scoped and well tested.

What looks good

  • Parser. parseLambdaBody parses the body with parseExpression(), so a lambda binds looser than every other operator. The check sits inside the non-PostgreSQL JSON loop, so other paths pay nothing. Rejecting ->> at parse time in ClickHouse and Snowflake is consistent with the retarget refusal.
  • Retargeting. binaryTargetForm now takes lambda, and every caller passes it (rewriteBitwise, nestsOutput, rewrite).
    • Lambdas are refused in dialects without them.
    • JSON -> and ->> are refused in ClickHouse and Snowflake.
    • The generic dialect keeps both.
    • targetExpressionPrecedence returns 0 for a lambda only when the target has lambdas.
  • Tokenizer. The new switch is only entered on the rare U&' or U&" path. PostgreSQL and the dialects that have & are unchanged. The E4001 error carries context and a hint.
  • Tests. The tests cover round trips, every refusal pair and the negative tokenizer cases. The golden JSON change is limited to the oracle, snowflake and clickhouse U& inputs.

Minor observations (non-blocking)

  1. No validation of lambda parameters. parseLambdaBody(left) accepts any left. So 2 * x -> x + 1 parses as 2 * (x -> x + 1), and a.b -> c or f(x) -> c parse as lambdas. Both dialects reject these anyway, so this only affects the quality of the error. Checking that left is an identifier or a tuple of identifiers would reject them earlier.
  2. Early return. The return in the loop is correct, because the body consumes everything. A one-line comment saying so would help the next reader.
  3. Duplicated dialect rule. Parser.arrowIsLambda and ast.lambdaTarget encode the same rule. The comment cross-references them. A small test asserting that they agree would catch drift when a dialect is added.
  4. Docs. Oracle, Snowflake and ClickHouse now hard-fail on U&'...'. The CHANGELOG covers this. A line in docs/SQL_COMPATIBILITY.md would help too.
  5. AST JSON dumps gain "Lambda". Check for any golden AST JSON outside the files touched here.

Overall this looks good to merge once CI is green.

…se it in CHANGELOG

ClickHouse/Snowflake -> generic now prints arrayMap(x -> (x * 2), arr)
instead of arrayMap(x -> x * 2, arr), which the generic parser read as
(x -> x) * 2. Pin all lambda statements through the public Transpile API
and list the output change in the #897 CHANGELOG entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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


Review of PR #901: ClickHouse/Snowflake lambdas and U& in dialects without &

I read the diff for the parser, tokenizer, AST and retargeting code. I did not run the tests. The PR description reports task check, cbinding and compatibility passing.

Verdict: I found no blocking issues. The change is small, well scoped and consistent with existing patterns.

What looks good

  • Parser (parseJSONExpression, parseLambdaBody):
    • The body is parsed with parseExpression(), so the lambda binds looser than every other operator.
    • The check sits inside the non-PostgreSQL JSON loop, so other dialects pay nothing for it.
    • ->> is rejected at parse time in ClickHouse and Snowflake. This keeps same-dialect transpiles from failing later with a confusing "no equivalent in " error.
  • Retargeting:
    • binaryTargetForm now takes lambda, and all three callers pass it (rewriteBitwise, nestsOutput, rewrite).
    • Lambdas are refused in dialects without them. JSON -> and ->> are refused in ClickHouse and Snowflake. The generic dialect keeps both.
    • targetExpressionPrecedence returns 0 for a lambda only when the target has lambdas.
  • Tokenizer: the switch is only entered on the rare U&' or U&" path. PostgreSQL and the dialects that have & are unchanged. The E4001 error carries context and a hint.
  • Tests:
    • They cover round trips, every refusal pair and the negative tokenizer cases.
    • The golden JSON change is limited to the oracle, snowflake and clickhouse U& inputs.

Minor observations (non-blocking)

  1. Lambda parameters are not validated. parseLambdaBody(left) accepts any left. So a.b -> c and f(x) -> c parse as lambdas, and 2 * x -> x + 1 parses as 2 * (x -> x + 1). Both dialects reject these anyway, so only the error quality is affected. Checking that left is an identifier or a tuple of identifiers would reject them earlier.
  2. Early return. The return inside the loop at pkg/sql/parser/expressions.go is correct, because the body consumes everything. A one-line comment saying so would help the next reader.
  3. Duplicated dialect rule. Parser.arrowIsLambda and ast.lambdaTarget encode the same ClickHouse/Snowflake rule. The comment cross-references them. A small test asserting that they agree would catch drift when a dialect is added.
  4. Docs. Oracle, Snowflake and ClickHouse now hard-fail on U&'...'. The CHANGELOG covers this. A line in docs/SQL_COMPATIBILITY.md would help too.
  5. AST JSON dumps gain "Lambda". Check for any golden AST JSON outside the files touched here.

Overall this looks good to merge once CI is green.

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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


Review of PR #901: ClickHouse/Snowflake lambdas and U& in dialects without &

I read the diff for the parser, tokenizer, AST and retargeting code. I did not run the tests. The PR description reports task check, cbinding and compatibility passing.

Verdict: I found no blocking issues. The change is small, well scoped and consistent with existing patterns.

What looks good

  • Parser (parseJSONExpression, parseLambdaBody):
    • The body is parsed with parseExpression(), so the lambda binds looser than every other operator.
    • The check sits inside the non-PostgreSQL JSON loop, so other dialects pay nothing.
    • ->> is rejected at parse time in ClickHouse and Snowflake. That avoids a confusing "no equivalent in " error on same-dialect transpiles.
  • Retargeting:
    • binaryTargetForm takes lambda, and all three callers pass it.
    • Lambdas are refused in dialects without them. JSON -> and ->> are refused in ClickHouse and Snowflake. The generic dialect keeps both.
    • targetExpressionPrecedence returns 0 only when the target has lambdas.
  • Tokenizer: the switch is only entered on the rare U&' or U&" path. PostgreSQL and the dialects that have & are unchanged. The E4001 error carries context and a hint.
  • Tests: they cover round trips, every refusal pair and the negative tokenizer cases. The golden JSON change is limited to U& inputs for oracle, snowflake and clickhouse.

Minor observations (non-blocking)

  1. Lambda parameters are not validated. parseLambdaBody(left) accepts any left. So a.b -> c and f(x) -> c parse as lambdas, and 2 * x -> x + 1 parses as 2 * (x -> x + 1). Both dialects reject these anyway, so only the error quality is affected. Checking that left is an identifier or a tuple of identifiers would reject them earlier.
  2. Early return. The return inside the loop in pkg/sql/parser/expressions.go is correct, because the body consumes everything. A one-line comment saying so would help the next reader.
  3. Duplicated dialect rule. Parser.arrowIsLambda and ast.lambdaTarget encode the same ClickHouse/Snowflake rule. The comments cross-reference them. A small test asserting that they agree would catch drift when a dialect is added.
  4. Docs. Oracle, Snowflake and ClickHouse now hard-fail on U&'...'. The CHANGELOG covers this. A line in docs/SQL_COMPATIBILITY.md would help too.
  5. AST JSON dumps gain "Lambda". Check for any golden AST JSON outside the files touched here.

Overall this looks good to merge once CI is green.

@ajitpratap0
ajitpratap0 merged commit 9784397 into main Oct 31, 2026
43 checks passed
@ajitpratap0
ajitpratap0 deleted the perf/w0-00a-transpile-correctness branch October 31, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ast AST node changes documentation Improvements or additions to documentation parser tokenizer Tokenizer / lexer changes

Projects

None yet

1 participant