Customizable serialization of PostgresType #965
Conversation
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 is useful for the pgx extension I work on, since we use a non-CBOR serialization for our aggregate states (Bincode) and it would be great if we were able to use |
|
hey @yrashk! this fell off my radar somehow. I think it looks good in general. Would you mind rebasing so we can see it without any conflicts? I'd like to merge if we can |
|
Absolutely! Will look into this shortly. |
However, all `Serialize/Deserialize` PostgresTypes are forced to use it with the blanket implementation of `IntoDatum`/`FromDatum` Solution: extract a `Serializer` trait to abstract this away By default, it uses CBOR and encodes it into a Varlena, replicating existing behavior. However, this gives a good amount of flexibility to end-user to determine their encoding needs. I've also removed JSON-encoding/decoding functions from varlena module, as they don't seem to be used anywhere.
Makes it unclear whether it's advisable to use it externally. Solution: document it
It takes care of both serialization and deserialization. However, some types can potentially be only deserialized, or serialized. Besides, the naming of the trait itself showed that it is not a perfect match. Solution: split it into Serializer and Deserializer, respectively
yrashk
force-pushed
the
custom-serialization
branch
from
fe084cf
to
cd26d68
Compare
|
Here's my first take on rebasing. It still requires a bit more of a review as things have changed and I may have missed something. The test passes. |
For all we know, it may still be using CBOR. Solution: ensure we're getting our custom serialization
yrashk
force-pushed
the
custom-serialization
branch
from
ba88d7d
to
9153f8f
Compare
|
@eeeebbbbrrrr did you have a chance to look at this updated PR yet? |
|
I have not. :( I’ll tackle it next week. Thanks for the ping. |
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.
Problem: CBOR is not the most universal answer to everything
However, all
Serialize/DeserializePostgresTypes are forced to use with the blanket implementation ofIntoDatum/FromDatumSolution: extract a
SerializerandDeserializertraits to abstract this awayBy default, it uses CBOR and encodes it into a Varlena, replicating existing behavior.
However, this gives a good amount of flexibility to end-user to determine their encoding needs.
I've also removed JSON-encoding/decoding functions from varlena module, as they don't seem to be used anywhere.
A message to pgx users: my ability to actively contribute to pgx largely depends on your support. Please consider sponsoring my work