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

add support for interval #10

Merged
merged 6 commits into from Aug 21, 2021
Merged

add support for interval #10

merged 6 commits into from Aug 21, 2021

Conversation

lslv1243
Copy link
Contributor

Very good lib!
I added support for interval type to make it better, I hope it helps others.
Interval is represented as Duration in dart.

@isoos
Copy link
Owner

isoos commented Aug 20, 2021

This looks great! Could you please also add:

  • a test for inserting and querying (may amend an existing one, although they tend to be too large to follow in the argument lists)
  • changelog entry in a similar format than the others?

@lslv1243
Copy link
Contributor Author

@isoos Do I create a new version for the changelog?

@isoos
Copy link
Owner

isoos commented Aug 20, 2021

Do I create a new version for the changelog?

yes, please do! e.g. 2.4.1 would be I think the appropriate, as there is no new dependency or major change

@lslv1243
Copy link
Contributor Author

@isoos I don't know how to run the tests locally, can you check if my tests are correct?

@isoos
Copy link
Owner

isoos commented Aug 21, 2021

I've run the CI tests and this is the failure there: PostgreSQLSeverity.error 08P01: insufficient data left in message

You can create a local docker image similar to this specification to run the tests:
https://github.com/isoos/postgresql-dart/blob/master/.github/workflows/dart.yml#L19-L24

@lslv1243
Copy link
Contributor Author

lslv1243 commented Aug 21, 2021

@isoos I can make it work by increasing the size of the ByteData to 16 instead of 8 when encoding (to match https://github.com/postgres/postgres/blob/master/src/include/catalog/pg_type.dat). But when I write I only set the first 8 bytes (bd.setInt64(0, value.inMicroseconds);).
Do you know what could be the next 8 bytes? Negative values are already working.

@isoos
Copy link
Owner

isoos commented Aug 21, 2021

Looks like the interval can store -178000000 years - 178000000 years in postgres in 16 bytes:
https://www.postgresql.org/docs/13/datatype-datetime.html

Let's increase the buffer size to 16, that's one step closer to the correct solution. However, I think we should also handle the second 8 bytes (or throw exception if it is non-null (*) in parsing the value, and also at encoding, if it is negative interval or larger than 8 bytes).

(*) I mean not all-zero.

@lslv1243
Copy link
Contributor Author

@isoos What do you think of the implementation now? It do can handle negative values, I don't know what would go in the second part.
I am throwing not implemented when decoding. I don't think there is something to be done when encoding since the dart int type has 64 bits size which matches the limit.

@isoos
Copy link
Owner

isoos commented Aug 21, 2021

I've found a relevant discussion in the a Go postgresql driver: lib/pq#78

I think I'll try to read a few other implementators code, but if nothing comes up in the next few hours, I'll merge this as-is, and publish it soon after. Maybe an additional test case with very large negative duration would be more reassuring, but this looks good to me.

@isoos
Copy link
Owner

isoos commented Aug 21, 2021

perfect test, let's publish this! :)

@isoos isoos merged commit 1d74c21 into isoos:master Aug 21, 2021
@isoos
Copy link
Owner

isoos commented Aug 21, 2021

2.4.1 is published, thanks for the contribution!

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.

None yet

2 participants