Skip to content
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

Fixed double/real + added timestamp, numeric, date & json #338

Merged
merged 4 commits into from
May 18, 2024

Conversation

pst9354
Copy link
Contributor

@pst9354 pst9354 commented May 17, 2024

Attempt to add some missing comversions in PostgresTextDecoder.
Also corrected conversion for double & real.

Created after discussion about: PostgresTextDecoder

@isoos
Copy link
Owner

isoos commented May 17, 2024

@pst9354: This is a great start! Could you please address the analysis error (Dead code.) and lints?

Would it be a stretching goal to also add some tests for these decodings, e.g. a few SELECT x::CAST queries with simple query protocol that check on the values?

Also: could you add a note to the changelog and bump the version?

@pst9354
Copy link
Contributor Author

pst9354 commented May 17, 2024

@isoos I don't see any analysis errors:
image

As everyone else, I'm really short of time :-) But I will try to find some time during the weekend...

@pst9354
Copy link
Contributor Author

pst9354 commented May 17, 2024

Well I'm horrible at using git.

It seems that the latest version is not uploaded.

I'll check.

@pst9354
Copy link
Contributor Author

pst9354 commented May 17, 2024

I just commited some changes.
Hope it's correct...

@isoos
Copy link
Owner

isoos commented May 17, 2024

Added some tests + fixed a few things to match the tests. I don't have the time to implement interval right now, but added a placeholder test for it too. If you are happy with the current state, I'll merge and publish a new version.

@pst9354
Copy link
Contributor Author

pst9354 commented May 17, 2024

I think you actually broke some conversions, let me have a look tomorrow,

@isoos
Copy link
Owner

isoos commented May 17, 2024

I think you actually broke some conversions, let me have a look tomorrow,

If you change the test's queryMode to extended, it should return the same values, although on a different protocol. Understandable interval fails there, and otherwise only two of the DateTimes are different (only by the Z at the end - unsure if it is a bug in the simple or in the extended protocol).

@pst9354
Copy link
Contributor Author

pst9354 commented May 18, 2024

Yes, the Z at the end of DateTime is important.

We want to get the same results, using either the extended or simple protocol.
With your changes thay differ.

Example:
SELECT '{\"color\": \"black\", \"size\": [\"S\",\"M\",\"L\",\"XL\"]}'::jsonb,1,now()::timestamp without time zone,now()::date,'TEST', 3::double precision, interval '5 hours',14.4::numeric;

Extended:
[[{size: [S, M, L, XL], color: black}, 1, 2024-05-18 06:19:36.491719Z, 2024-05-18 00:00:00.000Z, TEST, 3.0, 18000000000 microseconds, 14.4]]

Simple (your version):
[[{size: [S, M, L, XL], color: black}, 1, 2024-05-18 06:19:56.719728, 2024-05-18 00:00:00.000, TEST, 3.0, Instance of 'UndecodedBytes', 14.4]]

Simple (the version I commited, I only reverted the timestamp and date types):
[[{size: [S, M, L, XL], color: black}, 1, 2024-05-18 06:28:35.579774Z, 2024-05-18 00:00:00.000Z, TEST, 3.0, Instance of 'UndecodedBytes', 14.4]]

@pst9354
Copy link
Contributor Author

pst9354 commented May 18, 2024

I just pushed the changes needed to get same results in extended and simple protocol.

(We are not using hre interval or numeric types in any of our projects, so I really don't know what types they are supposed to return)

@isoos isoos force-pushed the Added-some-types-to-PostgresTextEncoder branch from b37b1d6 to 57c05d1 Compare May 18, 2024 07:48
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.11765% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 87.26%. Comparing base (12e1cbf) to head (57c05d1).
Report is 4 commits behind head on master.

Files Patch % Lines
lib/src/types/text_codec.dart 94.11% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   86.60%   87.26%   +0.65%     
==========================================
  Files          28       28              
  Lines        2964     2983      +19     
==========================================
+ Hits         2567     2603      +36     
+ Misses        397      380      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@isoos
Copy link
Owner

isoos commented May 18, 2024

I've tried to read more about this, and it is not clear to me what is being sent on the binary protocol, when somebody uses a timestamp without timezone. Filed #339 for further tracking of this issue, for the time being, I've opted to follow the binary protocol's values.

However, I updated your commit, as appending the Z marker seems to be fragile: the server may return the string value in different formats, where the Z postfix may not make sense, and in such case the entire parsing fails. With the current code it has a chance that DateTime.parse may still recognize it.

@isoos isoos merged commit 917fd32 into isoos:master May 18, 2024
1 check passed
@isoos
Copy link
Owner

isoos commented May 18, 2024

published, thank you!

@pst9354
Copy link
Contributor Author

pst9354 commented May 18, 2024

Great, my test-case works with the latest commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants