Skip to content

fix(ast): preserve IS NOT NULL and ClickHouse parametric aggregate params in output - #529

Open
basili4-1982 wants to merge 2 commits into
ajitpratap0:mainfrom
basili4-1982:fix/query-render-ast-losses
Open

fix(ast): preserve IS NOT NULL and ClickHouse parametric aggregate params in output#529
basili4-1982 wants to merge 2 commits into
ajitpratap0:mainfrom
basili4-1982:fix/query-render-ast-losses

Conversation

@basili4-1982

@basili4-1982 basili4-1982 commented Aug 21, 2026

Copy link
Copy Markdown

Summary

Two separate renderer bugs cause silent data-loss when serializing a parsed
query back to SQL via tree.SQL() / ast.*.SQL():

1. IS NOT NULL renders as IS NULL

The parser stores the operator as "IS NULL" and uses the Not flag to
disambiguate IS NOT NULL (pkg/sql/parser/expressions_operators.go).
BinaryExpression.SQL() (pkg/sql/ast/sql.go) returned the operator string
unchanged and ignored the Not flag in the null branch, inverting the
predicate. A query like WHERE email IS NOT NULL came back as
WHERE email IS NULL.

2. ClickHouse parametric aggregates drop their params group

FunctionCall.SQL() rendered arguments but never emitted Parameters
(field added for ClickHouse fn(params)(args) in #487), turning
quantileTDigest(0.95)(value) into quantileTDigest(value) — the 0.95
parameter was lost on round-trip.

Changes

  • pkg/sql/ast/sql.go:
    • BinaryExpression.SQL(): honour Not in the IS NULL branch.
    • FunctionCall.SQL(): render Parameters in their own parenthesis group
      before arguments.
  • pkg/sql/ast/is_not_null_test.go: new regression test (exact SQL assert).
  • pkg/sql/ast/roundtrip_test.go: added IS NOT NULL roundtrip case.
  • pkg/sql/parser/clickhouse_parametric_test.go: new
    TestClickHouseParametricAggregates_Render regression test.

Test plan

  • go test -short ./pkg/sql/ast/ ./pkg/sql/parser/ ./pkg/gosqlx/ — all green
  • New regression tests fail before the fix, pass after
  • Roundtrip (SELECT ... WHERE email IS NOT NULL) idempotent
  • ClickHouse quantileTDigest(0.95)(value) round-trips exactly

Note: TestPerformanceRegression fails in this environment against its
baseline (5 cases, SimpleSelect/ComplexQuery/WindowFunction/CTE/INSERT) — all
unrelated to these changes; the degraded queries touch none of the modified
code paths.
Fixes #530 (IS NOT NULL) and #531 (ClickHouse parametric aggregates params).

The parser stores `IS NULL` with Not=true for `IS NOT NULL`, but
BinaryExpression.SQL() ignored the Not flag in the null branch, silently
inverting the predicate to IS NULL.

Regression: pkg/sql/ast/is_not_null_test.go, plus a roundtrip case.
FunctionCall.SQL() emitted args but dropped the Parameters group, turning
quantile(0.5)(x) into quantile(x). Parameters are now rendered in their own
parenthesis group before args.

Regression: TestClickHouseParametricAggregates_Render in
pkg/sql/parser/clickhouse_parametric_test.go.
@vercel

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown

@basili4-1982 is attempting to deploy a commit to the coolajitpratapsingh0-9932's projects Team on Vercel.

A member of the Team first needs to authorize it.

@basili4-1982

Copy link
Copy Markdown
Author

Проверено, готово к мержу.

  • mergeable: MERGEABLE — без конфликтов с main (main ещё на 31b5b1c).
  • Тесты зелёные: go test -short ./pkg/sql/ast/ ./pkg/sql/parser/ ./pkg/gosqlx/ (включая новые TestRenderIsNotNullPreservesNot и TestClickHouseParametricAggregates_Render).
  • Единственный красный check — Vercel (Authorization required to deploy): это деплой форка, не связан с кодом; требует авторизации со стороны мейнтейнера. Apply labels и GitGuardian проходят.

Прошу ревью/мерж. Если Vercel блокирует — можно задеплоить из ветки репо или исключить его для PR-форков.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ast AST node changes parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] IS NOT NULL renders as IS NULL (NOT dropped on round-trip)

1 participant