Repository navigation
perf: do not create a new parser for each column - #383
Conversation
Convert a Spanner value directly to the PostgreSQL equivalent without creating a Parser object. This reduces the amount of memory that is allocated and must be garbage collected for each row that is sent to the PostgreSQL client.
Codecov Report
@@ Coverage Diff @@
## postgresql-dialect #383 +/- ##
========================================================
- Coverage 86.41% 86.24% -0.17%
- Complexity 1847 1885 +38
========================================================
Files 122 122
Lines 6064 6129 +65
Branches 838 839 +1
========================================================
+ Hits 5240 5286 +46
- Misses 592 603 +11
- Partials 232 240 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| static { | ||
| ByteConverter.bool(TRUE_VALUE_BYTES_BINARY, 0, true); | ||
| ByteConverter.bool(FALSE_VALUE_BYTES_BINARY, 0, true); |
There was a problem hiding this comment.
Why are we passing true as the third parameter here? Shouldn't it be false?
There was a problem hiding this comment.
Great catch, thanks! That's absolutely right, and it was not caught by our tests for two reasons:
- Most drivers will never use the binary format for booleans, because it does not really save any network traffic anyways (boolean values in text format are represented by
tandf, so they take up just one byte, just as the binary format of 1 and 0) - Our tests mostly used
trueas the test value, which meant that they did not catch the fact thatfalsewas encoded as true.
I've added additional tests that use randomly generated values so we can catch this kind of errors in the future. That testing also turned out to detect a bug in the PostgreSQL JDBC driver, as that driver does not correctly decode binary boolean values. I've filed an issue and a PR for the JDBC driver in their repository for it.
|
|
||
| public static byte[] convertToPG(ResultSet resultSet, int position, DataFormat format) { | ||
| switch (format) { | ||
| case SPANNER: |
There was a problem hiding this comment.
Why haven't we added the conversion logic for Cloud Spanner format for some of the data types?
There was a problem hiding this comment.
In Java, case statements automatically fall through, so here the same logic is used for both when the format is SPANNER and POSTGRESQL_TEXT. This is different from some other programming languages where this would mean that there is no conversion for SPANNER.
|
|
||
| public static byte[] convertToPG(ResultSet resultSet, int position, DataFormat format) { | ||
| switch (format) { | ||
| case SPANNER: |
There was a problem hiding this comment.
If the conversion to Spanner format is not possible for these data types, can we add some more specific error message rather than falling back to unknown data format?
There was a problem hiding this comment.
(See above)
The default case should not be reachable, but is required for the Java compiler, as it otherwise thinks that there are code paths possible that will not return a value for this method.
gauravsnj
left a comment
There was a problem hiding this comment.
LGTM with some small queries
Fix numeric parsing for NaN values. Fix bool binary parsing (always returned true). Add tests to verify that we actually receive the values we expect, both when using text format and when using binary format.
Convert a Spanner value directly to the PostgreSQL equivalent without creating a Parser object. This reduces the amount of memory that is allocated and must be garbage collected for each row that is sent to the PostgreSQL client.