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 Pgx_value_ptime with ptime converters #114

Merged
merged 7 commits into from
Jun 21, 2021

Conversation

jsthomas
Copy link
Contributor

This change addresses Issue #97 by introducing a new package called Pgx_value_ptime. This package is analogous to Pgx_value_core, but uses Ptime for date/date-time processing instead of Core.

The tests for Pgx_value_ptime are pretty much the same as those for Pgx_value_core, although I did add some extra tests for dates.

As I mentioned in the issue, I'm new to both this code base and OCaml in general, so please let me know if any of the code here isn't idiomatic and/or doesn't solve the problem the way you had in mind.

@jsthomas
Copy link
Contributor Author

This is failing in Circle with the error:

[ERROR] No solution for pgx & pgx_async & pgx_lwt & pgx_lwt_mirage & pgx_lwt_unix & pgx_unix & pgx_value_core & pgx_value_ptime: The following dependencies couldn't be met:
          - pgx_value_ptime -> ptime >= v0.8.3
              no matching version

I'm not sure why this is, opam seems to show Ptime 0.8.5 as available.

@brendanlong
Copy link
Contributor

I wonder if our build script is missing an opam update or something. I'll take a look.

Copy link
Contributor

@brendanlong brendanlong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great, thanks!

Let me try to figure out what's going on with CircleCI..

.circleci/config.yml Outdated Show resolved Hide resolved
Comment on lines +27 to +47
let to_time' text =
(*
The time string can come in various forms depending on whether the
Postgres timestamp used includes the time zone:

Without timezone
2016-06-07 15:37:46
2016-06-07 15:37:46.962425

With timezone
2016-06-07 15:37:46-04
2016-06-07 15:37:46.962425-04

For the first one we need to indicate that it's a UTC time by appending
a 'Z'. For the second one we need to append the minutes to the timezone.
*)
let open Re in
let tz = seq [ alt [ char '-'; char '+' ]; digit; digit ] in
let utctz = seq [ char 'Z'; eol ] |> compile in
let localtz = seq [ tz; char ':'; digit; digit; eol ] |> compile in
let localtz_no_min = seq [ tz; eol ] |> compile in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth pre-compiling the regexes as globals so we don't have to recompile them on each call. I'm not sure how slow that is in OCaml though so maybe it doesn't matter either way?

Suggested change
let to_time' text =
(*
The time string can come in various forms depending on whether the
Postgres timestamp used includes the time zone:
Without timezone
2016-06-07 15:37:46
2016-06-07 15:37:46.962425
With timezone
2016-06-07 15:37:46-04
2016-06-07 15:37:46.962425-04
For the first one we need to indicate that it's a UTC time by appending
a 'Z'. For the second one we need to append the minutes to the timezone.
*)
let open Re in
let tz = seq [ alt [ char '-'; char '+' ]; digit; digit ] in
let utctz = seq [ char 'Z'; eol ] |> compile in
let localtz = seq [ tz; char ':'; digit; digit; eol ] |> compile in
let localtz_no_min = seq [ tz; eol ] |> compile in
let to_time' =
(*
The time string can come in various forms depending on whether the
Postgres timestamp used includes the time zone:
Without timezone
2016-06-07 15:37:46
2016-06-07 15:37:46.962425
With timezone
2016-06-07 15:37:46-04
2016-06-07 15:37:46.962425-04
For the first one we need to indicate that it's a UTC time by appending
a 'Z'. For the second one we need to append the minutes to the timezone.
*)
let open Re in
let tz = seq [ alt [ char '-'; char '+' ]; digit; digit ] in
let utctz = seq [ char 'Z'; eol ] |> compile in
let localtz = seq [ tz; char ':'; digit; digit; eol ] |> compile in
let localtz_no_min = seq [ tz; eol ] |> compile in
fun text ->

@brendanlong brendanlong linked an issue Jun 21, 2021 that may be closed by this pull request
@brendanlong
Copy link
Contributor

Looks like we needed 0.8.3 instead of v0.8.3. I wonder if the opam maintainers would be interested in adding a warning for when this happens.

@jsthomas
Copy link
Contributor Author

Oops, thanks for figuring that out; I should've looked more closely and noticed the missing v.

@brendanlong
Copy link
Contributor

No worries :)

@brendanlong brendanlong merged commit 265516a into arenadotio:master Jun 21, 2021
@brendanlong
Copy link
Contributor

Oh I just noticed your comment asked for feedback on the code. I think it looked pretty good to me. One part I'm wondering about is if we could avoid the regexes somehow by using functions built into ptime, but I took a quick look and didn't see any. We might want to consider some performance optimizations around it, like pre-compiling the regexes or trying the regexes in sequence instead of in parallel (so we won't need to run all of them in most cases), but we can do improvements to the internals like that in separate commits if/when the performance matters.

Overall, I thought the PR looked great.

@jsthomas
Copy link
Contributor Author

Thanks for reviewing! This was a nice issue for learning more about how to use Ptime and about how postgres handles/presents date-times. The "good first issue" and "help wanted" labels were also helpful for finding something to work on.

yomimono pushed a commit to yomimono/pgx that referenced this pull request Mar 8, 2022
This change addresses issue arenadotio#97 by introducing a new package called Pgx_value_ptime. This package is analogous to Pgx_value_core, but uses Ptime for date/date-time processing instead of Core.

The tests for Pgx_value_ptime are pretty much the same as those for Pgx_value_core, although I did add some extra tests for dates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pgx_value_ptime with ptime converters
2 participants