fix(ast): preserve IS NOT NULL and ClickHouse parametric aggregate params in output - #529
Open
basili4-1982 wants to merge 2 commits into
Open
fix(ast): preserve IS NOT NULL and ClickHouse parametric aggregate params in output#529basili4-1982 wants to merge 2 commits into
basili4-1982 wants to merge 2 commits into
Conversation
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.
|
@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. |
This was referenced Aug 21, 2026
Author
|
Проверено, готово к мержу.
Прошу ревью/мерж. Если Vercel блокирует — можно задеплоить из ветки репо или исключить его для PR-форков. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 NULLrenders asIS NULLThe parser stores the operator as
"IS NULL"and uses theNotflag todisambiguate
IS NOT NULL(pkg/sql/parser/expressions_operators.go).BinaryExpression.SQL()(pkg/sql/ast/sql.go) returned the operator stringunchanged and ignored the
Notflag in the null branch, inverting thepredicate. A query like
WHERE email IS NOT NULLcame back asWHERE email IS NULL.2. ClickHouse parametric aggregates drop their params group
FunctionCall.SQL()rendered arguments but never emittedParameters(field added for ClickHouse
fn(params)(args)in #487), turningquantileTDigest(0.95)(value)intoquantileTDigest(value)— the0.95parameter was lost on round-trip.
Changes
pkg/sql/ast/sql.go:BinaryExpression.SQL(): honourNotin theIS NULLbranch.FunctionCall.SQL(): renderParametersin their own parenthesis groupbefore arguments.
pkg/sql/ast/is_not_null_test.go: new regression test (exact SQL assert).pkg/sql/ast/roundtrip_test.go: addedIS NOT NULLroundtrip case.pkg/sql/parser/clickhouse_parametric_test.go: newTestClickHouseParametricAggregates_Renderregression test.Test plan
go test -short ./pkg/sql/ast/ ./pkg/sql/parser/ ./pkg/gosqlx/— all greenSELECT ... WHERE email IS NOT NULL) idempotentquantileTDigest(0.95)(value)round-trips exactlyNote:
TestPerformanceRegressionfails in this environment against itsbaseline (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).