-
Notifications
You must be signed in to change notification settings - Fork 348
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
PostgreSQL Range Types #2008
PostgreSQL Range Types #2008
Conversation
Hello @tomohavvk, thank you for your thorough work! However, I would like to discuss some potential pitfalls I've found out in the PR (if I may). 1. Unintentional implicit conversionsTypedefs
I.e. all the provided conversions to and from Moreover, those implicits are not really necessary out there, compare please:
In other words, the implicit conversions there do not seem simplifying anything (nor improve clarity IMO) and in fact require even more lines of code to put everything together eventually. 2. Inexact type correspondenceSpeaking of the implicit val floatRangeMeta: Meta[Range[Float]] = rangeMeta("numrange")
implicit val doubleRangeMeta: Meta[Range[Double]] = rangeMeta("numrange")
implicit val bigDecimalRangeMeta: Meta[Range[BigDecimal]] = rangeMeta("numrange") AFAIK, CREATE TYPE floatrange AS RANGE (subtype = float8, ...); Then somewhere in user code:
So I would probably suggest to hold on creating too many type shortcuts from the beginning. Ultimately, if there is demand for that from users, it will be easier to add them later (comparing to deprecating and removing stuff in a case of complaints). 3. Over-backtickingIt is solely my personal perceptions, but does using a lot of back-ticked identifiers in the code really improves it? I feel it may make code more obscure instead. Especially because not all users like to see ASCII-art in their code. For example, I feel that more straightforward and less fancy naming could be more appreciated, e.g.: object Edge {
case object InclIncl extends Edge
case object InclExcl extends Edge
case object ExclIncl extends Edge
case object ExclExcl extends Edge
case object Empty extends Edge
} Also, the suggested approach allows to construct a Range like this: Range(Some(1), Some(2), Edge.empty) Which I am not sure about if it makes a lot of sense. |
Hello @satorg, thank you very much for the constructive comments, which have truly improved the implementation. I supported each of your suggestions, and here's what we have at the moment: 1. Unintentional implicit conversionsI completely removed any mentions of def rangeMeta[T](sqlRangeType: String)(encode: T => String, decode: String => T): Meta[Range[T]] 2. Inexact type correspondenceDone. 3. Over-backticking
I thought about it :) but it didn't stop me because in my opinion it was the easiest way to understand, when you actually look at those brackets that are in object Edge {
case object ExclExcl extends Edge
case object ExclIncl extends Edge
case object InclExcl extends Edge
case object InclIncl extends Edge
} 4. Empty range
I redesigned the range type with this in mind, now the range hierarchy looks like this: sealed trait Range[+T]
case object EmptyRange extends Range[Nothing]
case class NonEmptyRange[T](lowerBound: Option[T], upperBound: Option[T], edge: Edge) extends Range[T] We can still create a range like:
If there are any suggestions with improvements, I will be glad to consider them. |
Thanks for the fantastic PR description @tomohavvk and your review @satorg. I'll try and get this merged soon and be part of 1.0-RC6 |
Hollo community, I would like to introduce my vision for PostgreSQL range types implementation in doobie since it was requested in this issue but not implemented yet #936
I hope together we can improve this implementation and release the Range Types support into life
Range types
The following range types are supported, and map to doobie generic class:
int4range
schema type maps toRange[Int]
int8range
schema type maps toRange[Long]
numrange
schema type maps toRange[Float]
|Range[Double]
|Range[BigDecimal]
daterange
schema type maps toRange[java.time.LocalDate]
tsrange
schema type maps toRange[java.time.LocalDateTime]
tstzrange
schema type maps toRange[java.time.OffsetDateTime]
To control the inclusive and exclusive bounds according to the PostgreSQL specification you need to use a special
Edge
enumeration when creating aRange
:Note: the range types mappings are defined in a different object (
rangeimplicits
). To enable it you must import them explicitly:To create for example custom implementation of
Range[Byte]
you can use the public method which declared in the following packagedoobie.postgres.rangeimplicits
:The main requirements is to need to implement the
RangeBoundDecoder[Byte]
endRangeBoundEncoder[Byte]
which are essentially:For a
Range[Byte]
, the meta and bounds encoder and decoder would appear as follows: