-
Notifications
You must be signed in to change notification settings - Fork 840
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
EnableInfinityTs for encoding / decoding infinity to got time.Time type #1615
base: master
Are you sure you want to change the base?
Conversation
@@ -419,12 +419,17 @@ func (w timeWrapper) DateValue() (Date, error) { | |||
return Date{Time: time.Time(w), Valid: true}, nil | |||
} | |||
|
|||
func (w *timeWrapper) ScanTimestamp(v Timestamp) error { | |||
func (w *timeWrapper) ScanTimestamp(v Timestamp, infinityTsEnabled bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass down infinityTsEnabled from codec
@@ -276,7 +276,7 @@ func NewMap() *Map { | |||
m.RegisterType(&Type{Name: "text", OID: TextOID, Codec: TextCodec{}}) | |||
m.RegisterType(&Type{Name: "tid", OID: TIDOID, Codec: TIDCodec{}}) | |||
m.RegisterType(&Type{Name: "time", OID: TimeOID, Codec: TimeCodec{}}) | |||
m.RegisterType(&Type{Name: "timestamp", OID: TimestampOID, Codec: TimestampCodec{}}) | |||
m.RegisterType(&Type{Name: "timestamp", OID: TimestampOID, Codec: &TimestampCodec{}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to pointer so codec fields can be set by EnableInfinityTs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The are a few issues that need to be resolved before this can be merged. A few are inline with the code and the broader ones are mentioned here.
First, I don't understand:
Implemented only to TimestampCodec, TimestamptzCodec already handles infinity value
I don't see any code in TimestamptzCodec
that handles this mapping of infinite values to normal time.Time.
Whatever is done should be done for both.
Second, I don't think the Map
should have the EnableInfinityTs
method. The Map
type does not know anything about any Codec
implementations. It only works in terms of the Codec
interface. This violates that boundary. Instead, I would suggest the user of this feature call Map.RegisterType
with the properly configured Timestamp(tz)Codec
.
Next, do the values that Infinity
and -Infinity
are mapped to need to be configurable? I would have expected them to be hard-coded to the largest and smallest possible time.Time
s.
The TimestampScanner
interface can't be changed without breaking backwards compatibility. These conversions will actually have to occur in the Codec
s and/or ScanPlan
s and EncodePlan
s.
Lastly, I think we should find a better name for the whole feature than EnableInfinityTs
. That name makes sense in lib/pq where Infinity
wasn't supported at all. But Infinity
is already supported in pgx because it defines its own types instead of using time.Time
. This feature is clamping / converting values to fit in time.Time
. I would expect the feature / flag to be named something like (Clamp|Convert|Map)Infinity...
@@ -13,7 +13,7 @@ import ( | |||
const pgTimestampFormat = "2006-01-02 15:04:05.999999999" | |||
|
|||
type TimestampScanner interface { | |||
ScanTimestamp(v Timestamp) error | |||
ScanTimestamp(v Timestamp, infinityTsEnabled bool) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a breaking change. We can't do that in a without a major release.
infinityTsEnabled bool | ||
min time.Time | ||
max time.Time | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In might be more efficient to store a pointer to the Codec
rather than duplicate these fields.
} | ||
|
||
ts := m.nameToType["timestamp"] | ||
tsc, _ := ts.Codec.(*TimestampCodec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would panic if a different Codec
was registered.
This PR resolves #1574
Implemented only to TimestampCodec, TimestamptzCodec already handles infinity value
Changes:
pgtype.Map.EnableInfinityTs