-
Notifications
You must be signed in to change notification settings - Fork 777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: 🐛 Correctly switch coalesce()
result .NotNull
value
#1664
fix: 🐛 Correctly switch coalesce()
result .NotNull
value
#1664
Conversation
This change follows the previous implementation that mutates and returns the first column, but switches only the `.NotNull` value depending on the situation. BREAKING CHANGE: 🧨 `coalesce()` may assign nullable results ✅ Closes: sqlc-dev#1663
if firstColumn != nil { | ||
firstColumn.NotNull = shouldNotBeNull | ||
firstColumn.skipTableRequiredCheck = true | ||
cols = append(cols, firstColumn) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this really correct that using first column on this way?
if _, ok := arg.(*ast.A_Const); ok { | ||
shouldNotBeNull = true | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is it sufficient that asserting only A_Const
and ColumnRef
?
if firstColumn == nil { | ||
firstColumn = c | ||
} | ||
shouldNotBeNull = shouldNotBeNull || c.NotNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What are the differences between n.Args.Items
and their inner columns
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Sorry that it's take so long to review. Took me a few times through to understand the changes, but I think it's the right change and actually quite backwards compatible.
This change follows the previous implementation that mutates and returns the first column, but switches only the
.NotNull
value depending on the situation.A_Const
node may containNull
.Also, I am new to Golang OSS, so I may be making some large mistakes. Thank you in advance for your review.
BREAKING CHANGE: 🧨
coalesce()
may assign nullable results✅ Closes: #1663