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

Support Elixir durations #688

Open
josevalim opened this issue Jul 5, 2024 · 8 comments
Open

Support Elixir durations #688

josevalim opened this issue Jul 5, 2024 · 8 comments
Labels

Comments

@josevalim
Copy link
Member

Elixir version

1.17.0

Database and Version

Any

Postgrex Version

Not yet supported

Current behavior

Postgrex.Duration is not supported.

Expected behavior

We should support the new durations and potentially deprecate Postgrex.Duration.

@greg-rychlewski
Copy link
Member

For now were you just looking to encode Elixir durations? And then when we require 1.17 we can do the decoding/replace Postgrex.Duration?

@josevalim
Copy link
Member Author

I think that's a good beginning, yeah! We could maybe already even allow defaulting to them via an option?

@josevalim
Copy link
Member Author

This is in but we want to keep it around and eventually make it the default.

@endoooo
Copy link

endoooo commented Jan 31, 2025

after adjusting the config following the documentation (thanks to the tips in #706 and this Elixir Forum topic) everything was apparently working just fine.

I said apparently because while preparing a release, I noticed a strange behavior:

inspect in UI query in DB
Image Image

the DB interval is not matching the Duration in the application.

maybe this is somehow related to the notes described here?


relevant code

# config
config :my_app, MyApp.Repo, types: MyApp.PostgrexTypes

# my_app/postgrex_types.ex
Postgrex.Types.define(MyApp.PostgrexTypes, Ecto.Adapters.Postgres.extensions(),
  interval_decode_type: Duration
)

# migration
alter table(:table_name) do
  #...
  add :duration, :duration,
    generated: """
      ALWAYS AS (CASE
        WHEN closed_at IS NOT NULL THEN closed_at - inserted_at
        ELSE NULL
      END) STORED
    """
  #...
end

# schema
schema "students_records" do
  #...
  field :duration_until_close, :duration, read_after_writes: true
  #...
end

ps: sorry if this is not the right place to post it.

ps2: if someone can hint me with a starting point, I can try to investigate it and contribute with a PR (although, as a designer, my dev skills are limited haha)

@greg-rychlewski
Copy link
Member

Thanks @endoooo.

So the issue is that no matter how you insert the duration into Postgres, it will translate it internally into months, days, and microseconds. And that is how you will get the data back out of the database as well.

Given this, the current behaviour is not necessarily a bug but rather one representation choice out of many possible ones. The ones I can think of:

  • give the same units Postgres stores (months, days and microseconds) but then this won't be the same as the previous data type, Postgrex.Interval. we probably want some consistency between the two
  • make it exactly the same units as Postgrex.Interval (months, days, seconds, microseconds)
  • the current behaviour, which is try to fill the largest units first

I can see arguments for all 3....I don't think there is necessarily a clear cut answer. Given Postgrex.Interval has stood for quite a long time that was why I chose to basically do the same thing.

But let's see what @josevalim thinks.

@greg-rychlewski
Copy link
Member

Maybe the best compromise is this

make it exactly the same units as Postgrex.Interval (months, days, seconds, microseconds)

@josevalim
Copy link
Member Author

Yes, I think mirroring Postgrex.Interval is a good point 👍, and I also think that's the closest representation we can have to PostgreSQL itself, so we get 1 and 2.

@endoooo
Copy link

endoooo commented Feb 1, 2025

ahhhh... now I get it. thank you for the clarification @greg-rychlewski ! I knew it was some misconception on my part 😅

I was ready to create a PR with @josevalim suggestion, but I noticed you've already done this hehe

thank you guys!

endoooo added a commit to camino-school/lanttern that referenced this issue Feb 1, 2025
the `duration_until_close` field in `StudentRecord` is not matching the DB data. I've created an issue in Postgrex repo elixir-ecto/postgrex#688 and changed the implementation to calculate the duration on the fly

- created `render_days_and_hours/3` datetime helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants