add support for PostgreSQL range and multirange types#304
add support for PostgreSQL range and multirange types#304mamantoha wants to merge 2 commits intowill:masterfrom
Conversation
|
|
||
| describe "tsrange" do | ||
| test_decode "(lower,upper) - both exclusive", "'(2023-01-01 10:30:00,2023-12-31 15:45:00)'::tsrange", Time.utc(2023, 1, 1, 10, 30, 0)...Time.utc(2023, 12, 31, 15, 45, 0) | ||
| test_decode "(lower,upper] - exclusive lower, inclusive upper", "'(2023-01-01 10:30:00,2023-12-31 15:45:00]'::tsrange", Time.utc(2023, 1, 1, 10, 30, 0)..Time.utc(2023, 12, 31, 15, 45, 0) # Crystal can't represent exclusive lower |
There was a problem hiding this comment.
issue: Changing the exclusive lower to an inclusive one is a dangerous change.
Unfortunately, we cannot represent that with Range. But I'd consider silently converting the range to different semantics an error.
For Time we can work around by treating it as a discrete type. Just add one nanosecond:
| test_decode "(lower,upper] - exclusive lower, inclusive upper", "'(2023-01-01 10:30:00,2023-12-31 15:45:00]'::tsrange", Time.utc(2023, 1, 1, 10, 30, 0)..Time.utc(2023, 12, 31, 15, 45, 0) # Crystal can't represent exclusive lower | |
| test_decode "(lower,upper] - exclusive lower, inclusive upper", "'(2023-01-01 10:30:00,2023-12-31 15:45:00]'::tsrange", Time.utc(2023, 1, 1, 10, 30, 0, nanosecond: 1)..Time.utc(2023, 12, 31, 15, 45, 0) |
This doesn't preserve the exact semantics either, but it's much closer and preserves the intent.
And it probably won't work for PG::Numeric?
Alternatively, we need a different solution (e.g. a custom Range type).
Omitting this case (i.e. raise an error) would also be better than implicitly changing semantics.
There was a problem hiding this comment.
Right. Silently converting exclusive lower bounds to inclusive ones changes the semantics and could lead to bugs.
https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE
- Discrete types (
int4,int8,date): have clear "next" values (1 -> 2,2023-01-01->2023-01-02). This is working. - Continuous types (
timestamp,numeric): don't have clear "next" values.
From the documentation:
Even though timestamp has limited precision, and so could theoretically be treated as discrete, it's better to consider it continuous since the step size is normally not of interest.
Do you think I should implement these adjustments?
| test_decode "(lower,upper) - both exclusive", "'(1,10)'::int4range", 2...10 # PostgreSQL canonicalizes discrete ranges to [a,b) form | ||
| test_decode "(lower,upper] - exclusive lower, inclusive upper", "'(1,10]'::int4range", 2...11 | ||
| test_decode "[lower,upper) - inclusive lower, exclusive upper", "'[1,10)'::int4range", 1...10 | ||
| test_decode "[lower,upper] - both inclusive", "'[1,10]'::int4range", 1...11 # [a,b] becomes [a,b+1) for discrete types |
There was a problem hiding this comment.
question: Why not use inclusive Range here?
There was a problem hiding this comment.
https://www.postgresql.org/docs/current/rangetypes.html#RANGETYPES-DISCRETE
The built-in range types int4range, int8range, and daterange all use a canonical form that includes the lower bound and excludes the upper bound; that is, [). User-defined range types can use other conventions, however.
So when you write '[1,10]'::int4range in PostgreSQL:
- You specify
[1,10](both inclusive) - PostgreSQL internally converts it to
[1,11)(inclusive lower, exclusive upper) - The Crystal decoder receives this canonical
[1,11)form - Crystal represents it as
1...11(which is exactly[1,11))
There was a problem hiding this comment.
Understood, this normalization comes from the server 👍
2 participants
This PR adds support for PostgreSQL range and multirange types.
Features Added
int4range,int8range,daterange,tsrange,tstzrange,numrangeint4multirange,int8multirange,datemultirange,tsmultirange,tstzmultirange,nummultirange(PostgreSQL 14+)Testing
Breaking Changes
None - this is a pure addition of new functionality.