-
-
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
added support for json_strip_nulls
and jsonb_strip_nulls
#4255
added support for json_strip_nulls
and jsonb_strip_nulls
#4255
Conversation
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 opening this PR.
I added a few comments about the signatures of these functions. Additionally it would be great to have a test for json_strip_null(None)
to verify that the null value handling work.
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
fn jsonb_strip_nulls<E: MaybeNullableValue<Jsonb>>(jsonb: E) -> E::Out; |
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 need to restrict the generic type E
here to be either Jsonb
or Nullable<Jsonb>
. Also as we return the same output type as input type we don't need the MaybeNullableValue<Jsonb>
helper type:
fn jsonb_strip_nulls<E: MaybeNullableValue<Jsonb>>(jsonb: E) -> E::Out; | |
fn jsonb_strip_nulls<E: JsonbOrNullableJsonb>(jsonb: E) -> E; |
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
fn json_strip_nulls<E: MaybeNullableValue<Json>>(json: E) -> E::Out; |
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 need to restrict the generic type E
here to be either Json
or Nullable<Json>
. Also as we return the same output type as input type we don't need the MaybeNullableValue helper type. You likely need to define a new JsonOrNullableJson
trait analog to the JsonbOrNullableJsonb
trait for that.
fn json_strip_nulls<E: MaybeNullableValue<Json>>(json: E) -> E::Out; | |
fn json_strip_nulls<E: JsonOrNullableJson>(json: E) -> E; |
@weiznich Thank you sm for the review! I have pushed in a commit for the requested changes. Please let me know if it's alright or if there are any further changes. |
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.
Looks good, thanks for the update 🙏
cc0e359
to
5f5015e
Compare
Added support for functions
json_strip_nulls
andjsonb_strip_nulls
under #4216.