-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
impl pg array append #4138
Merged
Merged
impl pg array append #4138
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b4febd1
impl array_append
guissalustiano 8d53f2f
allow non nullable range
guissalustiano 444bab7
typo
guissalustiano 8ed4040
move cfg to outside the macro
guissalustiano e999560
remove nullable element
guissalustiano 263d1f1
add tests
guissalustiano 4f843d6
Switch the generic argument order to make it conistent with the real …
weiznich 56d979c
Fix running the doctests without time and numeric features
weiznich File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
We should have the following two properties (unless I'm mistaken on how postgresql behaves)
Can you add tests that showcase that both these things work as expected?
This should be doable by duplicating the existing test and explicitly specifying the types corresponding to each scenario.
Thanks 😊🙏
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 review!
The pg behavior for this functions with NULL is listed bellow:
SELECT array_append(ARRAY[1,2], 3)
returnsARRAY[1,2,3]
SELECT array_append(ARRAY[1,2], NULL)
returnsARRAY[1, 2, NULL]
SELECT array_append(NULL, 3)
returnsARRAY[3]
SELECT array_append(NULL, NULL)
returnsARRAY[NULL]
If passing NULL, postgres apparently handles it as an empty array, so I can't think of a way that this function returns NULL.
I see, thanks for the clarification. Can you help me on how to do that? That's why I did with
Nullable<T>
before, I don't know how to express this with types.Sure! Should I create doc tests or test in
diesel_test
?Thanks!
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.
Ah! Thanks for investigating the behavior! 😊
I thought that we may have to involve the nullability propagation helpers traits here to construct the return type, which would maybe have involved either declaring it without the macro or adding a feature to do so in the macro, however given what you're saying (3 and 4) it seems that we only ever want to return an array, regardless of original nullability of the array, so it seems that the current signature is correct. 😊
(Which is furtunate because that turns out to be significantly simpler.)
It's fine as a first version if a user needs the input array to already have its elements be Nullable to be able to append potentially null values to it. That may be incomplete (although I doubt there would be many use cases), but most importantly it's correct 😊.
What wouldn't have been fine is if it could return nulls but that wasn't expressed in the signature (1), or if by fixing (1) we accidentally made (2) wrong (by considering the arguments in the same way, in a mode where if any argument is Nullable resulting value is Nullable, which IIRC is already our default behavior for operators but apparently not for functions - what I meant is in that case we couldn't just "enable" that behavior for functions as well).
OK so overall, current implementation looks good, we can probably just add tests to make sure that we get the correct type for all 4 scenarios, with somewhat explicit sql types. 🙂
I have no strong opinion on where each test for the 4 scenarios you mentioned should live, but maybe another reviewer will. By default unless too verbose I'd probably just have them all in the one doctest.
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! I added the test as a docstring