fix(plan): allow parenthesized uuid default on string columns#24795
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking correctness issue:
The new ParenExpr handling unwraps parentheses only after the existing special-case NULL checks. That breaks the long-standing semantics that DEFAULT (NULL) should be equivalent to DEFAULT NULL.
In pkg/sql/plan/build_util.go, the JSON / GEOMETRY / NOT NULL guards still call isNullAstExpr(expr) before the new:
if paren, ok := expr.(*tree.ParenExpr); ok {
isExpressionDefault = true
expr = paren.Expr
}But isNullAstExpr() only recognizes a raw *tree.NumVal null literal. So DEFAULT (NULL) no longer hits those earlier NULL checks.
That creates real semantic regressions, for example:
NOT NULL DEFAULT (NULL)can bypass the existing invalid-null-default guard.- Types that explicitly allow only
DEFAULT NULLhere (such as JSON / GEOMETRY) will treat(NULL)as a non-NULL expression instead.
Please normalize/unwrap parenthesized expressions before the NULL-specific checks as well, while still preserving the original AST separately for OriginString.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Latest head fixes the earlier blocker around DEFAULT (NULL) by normalizing parenthesized expressions before the NULL-specific checks while still preserving the original default string. I did not find another blocking issue in the parenthesized DEFAULT (uuid()) handling on string columns.
Merge Queue Status
This pull request spent 1 hour 3 minutes 39 seconds in the queue, including 1 hour 2 minutes 43 seconds running CI. Required conditions to merge
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24734
What this PR does / why we need it:
Allows explicit parenthesized expression defaults such as
DEFAULT (uuid())on string columns, while preserving the existing UUID-type guard for bareDEFAULT uuid().Changes:
uuid()type guard.varchar NOT NULL DEFAULT (uuid()).Validation:
go test ./pkg/sql/plan -run 'TestBuildDefaultExprAllowsParenthesizedUuidForStringDefault|TestBuildDefaultExprKeepsBareUuidTypeGuard' -count=1git diff --check