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

Allow open-ended ranges, adjust defaults to default elixir behaviour #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marmor157
Copy link

Hello, I've taken the @maennchen's PR that's already exists #17, and I've added the tests you were asking for.

I've also adjusted the defaults of ranges. Now when the value is nil, unbound or empty the inclusive value is defaulted to false as it doesn't really makes sense for unbounded range to be inclusive.
I've also made so that if there's a value, the default inclusive is set to true to match Elixir's default behaviour of ranges being inclusive.

While doing so, I've discovered that postgres does not return the ranges exactly how they are stored elixir-ecto/postgrex#655. That means that sometime the input can be different than the output in terms of inclusiveness.

@maennchen
Copy link

Oh, ups. Completely forgot that I had an open PR. Thanks for taking care of this. I'll close my PR.

A few notes from my side:

  • It does make sense for unbound ranges to be inclusive, since they literally include everything on the unbound side.
  • Ranges do have a canonical representation in postgres. This is why you will get back different results than the ones you provide.

Also: Can you please either retain my commits or amend to them to preserve the author information?

@marmor157
Copy link
Author

marmor157 commented Oct 24, 2024

It does make sense for unbound ranges to be inclusive, since they literally include everything on the unbound side.

What I've meant by that, is that when you make range unbounded on either side, you include everything in that direction, so it does not make sense to differentiate between inclusive and exclusive. Or am I missing something here?

Ranges do have a canonical representation in postgres. This is why you will get back different results than the ones you provide.

Yes, but as stated in Postgrex.Range comment, what's written in the database is not returned identically in SQL query.
SELECT '(1,5)'::int4range returns [2,5).

Also: Can you please either retain my commits or amend to them to preserve the author information?

Yeah, sure, will do that soon. Vey sorry for that. I've copied your code just to test some things and then went on with just submitting the PR without giving it much thought.

@maennchen
Copy link

What I've meant by that, is that when you make range unbounded on either side, you include everything in that direction, so it does not make sense to differentiate between inclusive and exclusive. Or am I missing something here?

Ah no, they are also considered equivalent in postgres:
https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-INFINITE

SELECT '(1,5)'::int4range returns [2,5).

Yes, lower inclusive and upper exclusive is the canonical form for integer ranges.

https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE

@marmor157 marmor157 force-pushed the add-support-for-unbound-ranges branch from 853fb6c to 34aa158 Compare October 24, 2024 14:44
@vforgione
Copy link
Owner

vforgione commented Nov 30, 2024

@marmor157 I appreciate the effort here, but I'm not sure what this improves or fixes. I prefer what @maennchen did in his PR (#17) as it doesn't change the API and adheres to Postgres' range types explicitly.

Would you two be willing to reopen #17 and merge only the tests from this PR into that one?

@vforgione
Copy link
Owner

Noting that I updated the PR links in my previous comment. Sorry about the mix up.

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