Add proper tests for some types by seriyps · Pull Request #5 · tsloughter/pg_types

seriyps · 2019-09-30T23:50:18Z

Also, few bugs were fixed:

encoding and decoding of pg_date (needs verification over postgresql database)
integer uuid representation

Mostly some basic types covered.

seriyps · 2019-10-01T09:46:05Z

rebar3 cover -v
===> Verifying dependencies...
===> Compiling pg_types
===> Performing cover analysis...
  |------------------------|------------|
  |                module  |  coverage  |
  |------------------------|------------|
  |               pg_json  |      100%  |
  |              pg_range  |        0%  |
  |               pg_inet  |      100%  |
  |       pg_line_segment  |        0%  |
  |            pg_polygon  |        0%  |
  |               pg_int8  |      100%  |
  |               pg_uuid  |       81%  |
  |             pg_record  |        0%  |
  |              pg_jsonb  |      100%  |
  |             pg_circle  |        0%  |
  |               pg_name  |        0%  |
  |         pg_timestampz  |      100%  |
  |            pg_macaddr  |        0%  |
  |               pg_date  |      100%  |
  |               pg_time  |       76%  |
  |              pg_point  |        0%  |
  |          pg_timestamp  |       87%  |
  |                pg_raw  |        0%  |
  |               pg_path  |        0%  |
  |                pg_oid  |        0%  |
  |         pg_bit_string  |        0%  |
  |               pg_int2  |      100%  |
  |             pg_float4  |        0%  |
  |               pg_math  |        0%  |
  |               pg_enum  |        0%  |
  |           pg_interval  |        0%  |
  |            pg_boolean  |      100%  |
  |           pg_tsvector  |        0%  |
  |              pg_array  |        0%  |
  |               pg_line  |        0%  |
  |               pg_int4  |      100%  |
  |             pg_float8  |       30%  |
  |                pg_tid  |        0%  |
  |              pg_types  |        0%  |
  |             pg_hstore  |       83%  |
  |            pg_numeric  |        0%  |
  |------------------------|------------|
  |                 total  |       22%  |
  |------------------------|------------|

tsloughter · 2019-10-01T15:22:28Z

Woo, awesome!

* binaries * macaddr * name * tsvector * float extended

seriyps · 2019-10-01T23:12:06Z

Added some more:

  |------------------------|------------|
  |                module  |  coverage  |
  |------------------------|------------|
  |               pg_line  |      100%  |
  |                pg_oid  |        0%  |
  |           pg_tsvector  |      100%  |
  |               pg_math  |        0%  |
  |              pg_point  |      100%  |
  |            pg_boolean  |      100%  |
  |              pg_types  |        0%  |
  |               pg_json  |      100%  |
  |               pg_int8  |      100%  |
  |             pg_circle  |      100%  |
  |          pg_timestamp  |       87%  |
  |              pg_array  |        0%  |
  |                pg_tid  |        0%  |
  |               pg_path  |      100%  |
  |               pg_uuid  |       81%  |
  |               pg_enum  |        0%  |
  |             pg_record  |        0%  |
  |            pg_polygon  |      100%  |
  |               pg_inet  |      100%  |
  |             pg_float4  |        0%  |
  |              pg_jsonb  |      100%  |
  |             pg_float8  |       90%  |
  |               pg_date  |      100%  |
  |               pg_name  |       75%  |
  |             pg_hstore  |       83%  |
  |               pg_time  |       76%  |
  |            pg_macaddr  |      100%  |
  |               pg_int2  |      100%  |
  |                pg_raw  |      100%  |
  |         pg_bit_string  |      100%  |
  |               pg_int4  |      100%  |
  |            pg_numeric  |        0%  |
  |              pg_range  |        0%  |
  |           pg_interval  |        0%  |
  |       pg_line_segment  |      100%  |
  |         pg_timestampz  |      100%  |
  |------------------------|------------|
  |                 total  |       35%  |
  |------------------------|------------|

I think I'm done now.

pg_oid doesn't do any decoding, so, I'm not sure how to test it
pg_float4 has a truncation problem. I have some ideas on how to solve it, but not now.
pg_numeric - uses floats, so I expect it to be unreliable. I think representation of pg_numeric should be configurable (eg, see https://hex.pm/packages/epgsql_decimal)
Remaining ones depend on persistent_term

seriyps · 2019-10-02T10:25:08Z

src/pg_date.erl


 decode(<<Date:?int32>>, _TypeInfo) ->
-    calendar:gregorian_days_to_date(Date + ?POSTGRESQL_GD_EPOCH).
+    decode_date(Date + ?POSTGRESQL_GD_EPOCH).


would be nice to verify that over the real postgresql server. But that's how it's done in epgsql.

What do you mean "verify over the real postgresql server"?

I mean, we are converting datetime to some integer and send it to PostgreSQL; then we read it back as integer and convert back to datetime using the same algorithm. So, what we send and what we will receive will be the same, but what should be verified is that this integer is interpreted as the same YYYY-MM-DD_HH:mm:ss as in Erlang.
Say, we have {{2019, 11, 9}, {19, 59, 0}} in erlang. We encode it to some integer, send it to postgres, but what if postgres interprets this integer as some other date? Are we sure ituses gregorian_days_to_date?

seriyps · 2019-10-02T10:27:41Z

test/prop_float.erl

+%% TODO: fix truncation problem
+%% prop_float4_codec() ->
+%%     ?FORALL(Val, proper_types:float(-999.99, 990.99),
+%%             proper_lib:codec(pg_float4, [], Val, fun canonical_float4/1)).


Guess can be fixed by smth like:

float32_gen() -> ?LET(F, proper_types:float(), begin <<F1:32/float>> = <<F:32/float>>, F1 end).

so, truncation is done inside the generator, not inside the codec? Or is there any better solution?

seriyps · 2019-10-02T10:31:42Z

test/prop_int.erl

+
+prop_int2_codec() ->
+    ?FORALL(Val, proper_lib:int16(),
+            proper_lib:codec(pg_int2, [], Val)).


I'm thinking now maybe we should add a guards to integer encoders that ensure that the value is within the allowed range, instead of silently truncating when overflown?

Good point, agreed.

seriyps · 2019-10-02T10:33:15Z

test/prop_json.erl

+    proper_types:binary().
+
+
+json_struct() ->


I've tried to implement recursive generator for JSON, but failed to do so =) But it isn't needed really, because we are just doing term_to_binary anyway.

seriyps · 2019-10-02T13:08:29Z

test/prop_uuid.erl

+prop_integer_codec() ->
+    ?FORALL(Val, proper_types:integer(
+                   -170141183460469231731687303715884105728,
+                   170141183460469231731687303715884105728),


Should UUID be represented as signed integer?

Yea, though pg_types supports integer, string and binary.

I mean signed vs unsigned integer

tsloughter · 2019-11-09T18:57:06Z

Regarding numeric, I thought @starbelly already did code for handling numerics as tuples instead of floats... but I see it isn't in pg_numeric so maybe I lost it...

I'm still not sure what to do about persistent_term being used by types like array. I'm going to toy with recursive resolving today so that types don't have to be looked up in pg_array.

starbelly · 2019-11-09T19:17:23Z

@tsloughter hmmm I did, but then i think we decided not to include it. I can send up a PR which adds a clause for it though. The original clause was {float, weight, scale}, but that didn't jive with how numeric is implemented in postgres iirc. So are we talking about {float, weight, scale} or are we talking about something like {sign, coef, exp} ?

tsloughter · 2019-11-09T19:26:52Z

@seriyps so I have a fix for the use of persistent_term in array and record. It looks like the only catch is that there is some case that there aren't oids available in the record type and it decodes without them because they can be found in the data being decoded https://github.com/tsloughter/pg_types/blob/master/src/pg_record.erl#L67-L69

I don't know when that is an issue that you wouldn't have the oids in comp_oids and have to get them from the data as you decode... any idea?

Maybe it isn't something that actually has to be supported?

tsloughter · 2019-11-09T20:37:49Z

Crap, the record decode without knowing the oids before hand is used for tuples. I was just adding a test for SELECT (NULL, NULL) and it uses the decode_tuple_no_oids function.

tsloughter · 2024-11-12T18:46:50Z

Only took 5 years, hehe, but I merged part of this through a separate PR #9

seriyps · 2024-11-12T21:29:22Z

Cool, thanks!

starbelly · 2024-11-13T00:48:13Z

Regarding numeric, I thought @starbelly already did code for handling numerics as tuples instead of floats... but I see it isn't in pg_numeric so maybe I lost it...

You didn't lose it, it's here https://github.com/tsloughter/pg_types/blob/master/src/pg_numeric.erl

But you can't use tuples 😄 You'd have to add an abstraction layer, and maybe that's what you thought I did, instead at some point pgo was trying to use tuples in this way, but I did the work to fix up numerics originally, which was mostly carried over to this repo, here is the original PR to pgo

I'm still not sure what to do about persistent_term being used by types like array. I'm going to toy with recursive resolving today so that types don't have to be looked up in pg_array.

I'm not sure what the talk about persistent_term here is, but one does have to be very careful with it and also keep in mind that we don't know what people are doing with persistent_term in other libs that an app nor the app itself.

tsloughter · 2024-11-13T10:53:38Z

I'm not sure what the talk about persistent_term here is, but one does have to be very careful with it and also keep in mind that we don't know what people are doing with persistent_term in other libs that an app nor the app itself.

Here it is used to store type information for mapping oids to types.

I wanted to keep this library without having any dependency on the other library using it to pass anything but the binary postgres term and the type info for that term. This works everywhere but pg_record where there is apparently a case that comp_type is [].

I don't remember the details but I think we could fix this by changing the api to accept a map of types.

It just means pgo also has to change to update each connection pool record to have and update when types are added this map.

starbelly · 2024-11-16T21:48:18Z

I'm not sure what the talk about persistent_term here is, but one does have to be very careful with it and also keep in mind that we don't know what people are doing with persistent_term in other libs that an app nor the app itself.

Here it is used to store type information for mapping oids to types.

I wanted to keep this library without having any dependency on the other library using it to pass anything but the binary postgres term and the type info for that term. This works everywhere but pg_record where there is apparently a case that comp_type is [].

I don't remember the details but I think we could fix this by changing the api to accept a map of types.

It just means pgo also has to change to update each connection pool record to have and update when types are added this map.

Ahh, so, this would be stored in persistent_term and really never change except for hot code upgrades and restarts, is that correct?

tsloughter · 2024-11-19T10:50:40Z

@starbelly right.

seriyps added 2 commits October 1, 2019 01:44

Add proper tests for some types

96537a2

Add float64

994b075

seriyps mentioned this pull request Oct 1, 2019

Breakout datatypes to a separate library epgsql/epgsql#180

Open

seriyps added 2 commits October 2, 2019 00:10

Add tests for geometry types

893cf66

More types tests

acdd9f7

* binaries * macaddr * name * tsvector * float extended

seriyps commented Oct 2, 2019

View reviewed changes

Merge branch 'master' into proper-tests

3328d29

		proper_types:binary().


		json_struct() ->

Conversation

seriyps commented Sep 30, 2019

Uh oh!

seriyps commented Oct 1, 2019

Uh oh!

tsloughter commented Oct 1, 2019

Uh oh!

seriyps commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsloughter commented Nov 9, 2019

Uh oh!

starbelly commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsloughter commented Nov 9, 2019

Uh oh!

tsloughter commented Nov 9, 2019

Uh oh!

tsloughter commented Nov 12, 2024

Uh oh!

seriyps commented Nov 12, 2024

Uh oh!

starbelly commented Nov 13, 2024

Uh oh!

tsloughter commented Nov 13, 2024

Uh oh!

starbelly commented Nov 16, 2024

Uh oh!

tsloughter commented Nov 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seriyps commented Oct 1, 2019 •

edited

Loading

starbelly commented Nov 9, 2019 •

edited

Loading