Use PSQLFormat when encoding and decoding #158
Merged
Conversation
This file contains hidden or 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
fabianfett
changed the title
fabianfett
force-pushed
the
ff-use-format-when-decoding
branch
from
5a18b1c to
8029bcd
Compare
glbrntt
reviewed
May 13, 2021
Sources/PostgresNIO/New/Connection State Machine/ExtendedQueryStateMachine.swift
Outdated
Show resolved
Hide resolved
Comment on lines
+17
to
21
| case (.binary, .float8): | ||
| guard buffer.readableBytes == 8, let double = buffer.readDouble() else { | ||
| throw PSQLCastingError.failure(targetType: Self.self, type: type, postgresData: buffer, context: context) | ||
| } | ||
| return Float(double) |
There was a problem hiding this comment.
Are we okay with the float-to-double conversion?
There was a problem hiding this comment.
The implementation I inherited did this, so I guess it's okay for now. Maybe change with the next major release?
Comment on lines
+24
to
+26
| self.parameters.forEach { | ||
| buffer.writeInteger($0.psqlFormat.rawValue) | ||
| } |
There was a problem hiding this comment.
Should reserve capacity in the buffer to minimise intermediate allocs?
There was a problem hiding this comment.
Although we have to encode the actual parameters as well which make it harder to know how much capacity to reserve.
There was a problem hiding this comment.
Maybe in another pr at a later point?
Comment on lines
7
to
8
| /// In a `RowDescription` returned from the statement variant of `Describe`, | ||
| /// the format is not yet known and will always be `.text`. |
There was a problem hiding this comment.
I don't understand this, does it mean that we assume it to be .text until we know otherwise?
There was a problem hiding this comment.
Removed the sentence in question. this is explained in ExtendedQueryStateMachine much better. And is really only relevant there.
There was a problem hiding this comment.
fabianfett
force-pushed
the
ff-use-format-when-decoding
branch
2 times, most recently
from
6df8e99 to
82d57b3
Compare
fabianfett
commented
May 14, 2021
Comment on lines
22
to
33
| /// Decode an entity from the `byteBuffer` in postgres binary format | ||
| /// | ||
| /// - Parameters: | ||
| /// - byteBuffer: A `ByteBuffer` to decode. The byteBuffer is sliced in such a way that it is expected | ||
| /// that the complete buffer is consumed for decoding | ||
| /// - type: The postgres data type. Depending on this type the `byteBuffer`'s bytes need to be interpreted | ||
| /// in different ways. | ||
| /// - format: The postgres wire format. Can be `.text` or `.binary` | ||
| /// - context: A `PSQLDecodingContext` providing context for decoding. This includes a `JSONDecoder` | ||
| /// to use when decoding json and metadata to create better erros. | ||
| /// - Returns: A decoded object | ||
| static func decode(from byteBuffer: inout ByteBuffer, type: PSQLDataType, format: PSQLFormat, context: PSQLDecodingContext) throws -> Self |
There was a problem hiding this comment.
@glbrntt Does that make sense?
There was a problem hiding this comment.
Mostly! Aren't these are in conflict?
/// Decode an entity from the `byteBuffer` in postgres binary format /// - format: The postgres wire format. Can be `.text` or `.binary`
glbrntt
reviewed
May 14, 2021
| /// in different ways. | ||
| /// - format: The postgres wire format. Can be `.text` or `.binary` | ||
| /// - context: A `PSQLDecodingContext` providing context for decoding. This includes a `JSONDecoder` | ||
| /// to use when decoding json and metadata to create better erros. |
There was a problem hiding this comment.
Suggested change
| /// to use when decoding json and metadata to create better erros. | |
| /// to use when decoding json and metadata to create better errors. |
Comment on lines
22
to
33
| /// Decode an entity from the `byteBuffer` in postgres binary format | ||
| /// | ||
| /// - Parameters: | ||
| /// - byteBuffer: A `ByteBuffer` to decode. The byteBuffer is sliced in such a way that it is expected | ||
| /// that the complete buffer is consumed for decoding | ||
| /// - type: The postgres data type. Depending on this type the `byteBuffer`'s bytes need to be interpreted | ||
| /// in different ways. | ||
| /// - format: The postgres wire format. Can be `.text` or `.binary` | ||
| /// - context: A `PSQLDecodingContext` providing context for decoding. This includes a `JSONDecoder` | ||
| /// to use when decoding json and metadata to create better erros. | ||
| /// - Returns: A decoded object | ||
| static func decode(from byteBuffer: inout ByteBuffer, type: PSQLDataType, format: PSQLFormat, context: PSQLDecodingContext) throws -> Self |
There was a problem hiding this comment.
Mostly! Aren't these are in conflict?
/// Decode an entity from the `byteBuffer` in postgres binary format /// - format: The postgres wire format. Can be `.text` or `.binary`
Co-authored-by: George Barnett <[email protected]>
fabianfett
force-pushed
the
ff-use-format-when-decoding
branch
from
82d57b3 to
21fb161
Compare
glbrntt
approved these changes
Jul 22, 2021
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.
Postgres supports two encodings on the wire text (0) and binary (1). Up until now the new
PSQLDecodableonly supported binary encoding.PSQLFormatCodetoPSQLFormatPSQLFormatas parameter to the decode function.psqlFormat: PSQLFormatas protocol requirement forPSQLEncodable.binaryformat only (we can change this with follow up PRs)