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 ranges #213

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add support for ranges #213

wants to merge 5 commits into from

Conversation

pascalbetz
Copy link

@pascalbetz pascalbetz commented Oct 3, 2020

Implements Ranges
(#164)

This is as far as i can get with my limited Crystal knowhow...so i'll need some help to finish it.

@will
Copy link
Owner

will commented Oct 3, 2020

Thanks for putting this together. I'm not available for a couple of days, but I'll take a look at this next week and give feedback then :)

@pascalbetz
Copy link
Author

Begin/Endless Ranges

Postgres supports "infinite" values for lower/upper boundary. This corresponds to begin/endless Ranges in Crystal.
E.g.: [, 50) => nil..50
This makes the Range that gets created Range(T | Nil, T | Nil)
Is this OK?

Another possibility would be to use the Datatypes MIN/MAX value (e.g. Int64::MIN) and make sure the boundary is inclusive:
E.g.: [, 50) => Int32::MIN..50 so we would not create begin/endless Ranges. (This would not map what is in Postgres though, so I don't like it)

Empty Ranges

Postgres empty ranges are currently implemented as exclusive Ranges with the same value for lower and upper boundary.
E.g. 0...0

The values I picked to build these empty ranges are arbitrary though and this does not map exactly what is stored in Postgres.

There is no way to differentiate between

(,)::int4range and [0,0]::int4range because both values end up in Range.new(0, 0).

What do you think?

@pascalbetz
Copy link
Author

@will
I plan on fixing the build and adding multiranges (Postgres 14) support if this can get merged. Let me know what you think.

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