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 the postgres multirange type #4159

Merged
merged 9 commits into from
Aug 14, 2024

Conversation

guissalustiano
Copy link
Contributor

@guissalustiano guissalustiano commented Aug 12, 2024

This PR implements the Postgres multirange types

@guissalustiano guissalustiano force-pushed the pg_multirange branch 2 times, most recently from 5eed8e3 to 13b453a Compare August 12, 2024 03:00
@weiznich weiznich requested a review from a team August 12, 2024 15:07
@guissalustiano
Copy link
Contributor Author

guissalustiano commented Aug 12, 2024

Tests fail on Windows because the minimum version for multirange is postgres 14, released on 09/2021, and windows install postgres 12

The other failures are related to the proptest

[quickcheck] TEST FAILED (runtime error). Arguments: ([(-210866803201, 0, 0, 0)])
Error: "Query failed: DatabaseError(Unknown, \"timestamp out of range\") -> value: [(Included(-4713-11-23T23:59:59), Excluded(1970-01-01T00:00:00))]"

I'm not sure how the protest for Range<Timestamp> works for this, but we need to reduce the test domain on these tests (or just test as vec![(Bound<T>, Bound<T>)].

And

[quickcheck] TEST FAILED. Arguments: ([(-211785610425, 0, -210004501573, 0), (-1, 0, 0, 0)])
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I don't have any idea why it fails but I would guess that is a db error again

@weiznich
Copy link
Member

For the windows CI error: Sounds like we just need to install a newer Postgres version there. It's fine to include that as part of the PR.

For the proptest error: I'm not at my computer but I will have a look in the next days and see what von be done there.

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR. That's really helpful work 🙏

I have some small note about the ToSql implementation + a minor question. Otherwise I would like to see an Changelog.md entry for this, as this is a relevant new feature. Otherwise this should be fine to merge.

diesel/src/pg/types/multirange.rs Outdated Show resolved Hide resolved
diesel/src/pg/types/multirange.rs Outdated Show resolved Hide resolved
@weiznich weiznich enabled auto-merge August 14, 2024 15:03
@weiznich weiznich added this pull request to the merge queue Aug 14, 2024
Merged via the queue into diesel-rs:master with commit c85da9c Aug 14, 2024
49 checks passed
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.

2 participants