feat: bootstrap jiff-sqlx development #141

tisonkun · 2024-10-11T19:18:44Z

This refers to #50.

Firstly, I considered to add a separated sqlx-jiff method, but soon I found that due to Rust's orphan rules, implementing sqlx's trait for Timestamp and other types requires the code to be located at the same crate (jiff). Otherwise we need an extra wrapper struct like:

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct Timestamp(pub jiff::Timestamp);

I prefer to implement code in jiff and gate it with feature flags.

@BurntSushi This demonstrates the integration implementation direction. I'll complete other parts when we agree on this direction.

Signed-off-by: tison <wander4096@gmail.com>

BurntSushi · 2024-10-11T19:32:41Z

#50 (comment) discusses this. And the orphan rule is taken into account there. The way to do this integration, short of getting it merged upstream in sqlx itself, is with a wrapper type in a jiff-sqlx crate. The comment linked explains how to do this.

Jiff is a "low level" crate. Having it depend on "high level" crates like sqlx would lead to all sorts of problems, especially if and when the time ever comes for sqlx to add Jiff integration on their end. It's really a non-starter.

This reverts commit 89f6330.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun

@BurntSushi Yeah .. Adopt the other way now.

sqlx-jiff/src/lib.rs

BurntSushi · 2024-10-11T19:58:51Z

Also, the crate should be called jiff-sqlx, since it lives with jiff.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2024-10-11T21:01:50Z

Other type mapping to be wrapped once we agree on the pattern:

jiff::civil::Date <-> DATE
jiff::civil:Time <-> TIME
jiff:civil::DateTime <-> TIMESTAMP
jiff::Timestamp <-> TIMESTAMPTZ
jiff::SignedDuration <-> INTERVAL

Ref -

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2024-10-23T07:21:38Z

All bridges submitted. Note that integrations for PgRange and PgTimeTz cannot be added due to the orphan rule.

I'd suspend the task here to start jiff-sqlx with Postgres support first since it's my real use case and I'd avoid adding too many things at the very beginning.

tisonkun · 2024-10-23T11:22:36Z

The failing wasm CI job doesn't seem to be relevant to this patch ..

Signed-off-by: tison <wander4096@gmail.com>

maxcountryman · 2024-11-13T00:26:58Z

jiff::SignedDuration <-> INTERVAL

I'm curious why this wouldn't be jiff::Span? That's how I'm doing these conversions manually.

tisonkun · 2024-11-13T15:23:23Z

@maxcountryman

It's possible to support multiple type mappings, so it's possible to support both Span and Duration.

However, PG's interval supports only months, days and microseconds parts. Span supports other units. I'm not sure if we can always assume 1 year=12 months, and 1 week=7days.

Anyway, we may support two types together.

maxcountryman · 2024-11-15T18:03:05Z

Span supports other units. I'm not sure if we can always assume 1 year=12 months, and 1 week=7days.

As I understand, Span doesn't assume 12 months == 1 year (ergo keeping each unit as a discrete property). I think that might be a reason to use Span here, right? (In particular, I believe Span is needed for full ISO 8601 support.)

jiff-sqlx/Cargo.toml

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2025-01-04T05:17:42Z

Hi @BurntSushi!

IMO code gets evolved and mature by tested in real world.

It's understandable that your context is not for such an integration, and you don't want release code that you don't look carefully into.

So, I wonder if it's OK for you that I release this crate under the name jiff-sqlx while I'm the crate owner for all the followups. And once you or the sqlx's maintainer accept to maintain it in either upstream (for the benefits to evolve together and catch up changes in the first place), we can work a transfer solution then.

BurntSushi · 2025-01-04T05:25:20Z

Can you please pick a different name? That way we don't need to do any transfers or have any other project expectations.

I also thought you had this code in your own project internally? Why do you need to publish a crate to do it?

tisonkun · 2025-01-04T05:40:00Z

Can you please pick a different name? That way we don't need to do any transfers or have any other project expectations.

Then I'd leave this PR here so that anyone need it can use it by copying the code. I don't think such an integration worth a dedicated name.

I also thought you had this code in your own project internally? Why do you need to publish a crate to do it?

This is somehow like a question "why do you write opensource code". I write opensource code everyday so sharing code is by default (as shown in my GitHub profile). Also you can see that there are several other users want it. This is enough.

BurntSushi · 2025-01-04T16:50:10Z

Yes sorry, it is totally cool to publish a crate for the good of open source. I was in a "are you yourself blocked" frame of mind versus "you want to publish a crate for others to use." So in the latter case, I think it's just a matter of either you going ahead and doing it (which is your right, although I request that you not use a jiff- prefix) or waiting until I'm able to dig into it.

Ecosystem integration is a big problem and I need to context switch into it. I'm just not there yet. And I'm currently thinking I'll do it with a jiff 0.2 release, since there are a number of breaking changes I'd like to make.

maxcountryman · 2025-01-04T17:04:07Z

My two cents: this is not the correct implementation re Postgres intervals. I think that should be addressed before publishing.

BurntSushi · 2025-01-04T17:11:48Z

@maxcountryman I haven't looked too closely yet. It looks like only SignedDuration is supported here and it collapses everything down to microseconds. Can you say more about what you see as incorrect?

maxcountryman · 2025-01-04T17:18:22Z

@BurntSushi that doesn't support ISO 8601 correct? See: https://www.postgresql.org/docs/13/datatype-datetime.html#DATATYPE-INTERVAL-OUTPUT

BurntSushi · 2025-01-04T17:23:09Z

SignedDuration would still certainly emit ISO 8601.

Regardless, I'll dig into this more when I address ecosystem integration. But if you have any more explanation, I would be happy to hear it!

maxcountryman · 2025-01-04T17:31:27Z

@BurntSushi This passage in the docs: "Unlike the Span type, though, only uniform units are supported. This means that ISO 8601 durations with non-zero units of days or greater cannot be parsed directly into a SignedDuration" along with this from the Postgres docs, "As previously explained, PostgreSQL stores interval values as months, days, and microseconds" seem incompatible.

BurntSushi · 2025-01-04T17:34:40Z

jiff-sqlx/src/postgres/interval.rs

+        value: <Postgres as Database>::ValueRef<'r>,
+    ) -> Result<Self, BoxDynError> {
+        let pg_interval = PgInterval::decode(value)?;
+        let micros = pg_interval.microseconds;


@maxcountryman Ah so I think this is where there is a correctness issue? The non-microseconds units are being silently ignored here.

This is almost certainly incorrect, yes.

However, I'm also pointing out that Postgres can be configured to output ISO 8601. Your docs say SignedDuration is not capable of parsing ISO 8601 with non-zero days. Moreover, my impression is that Spans may be preferred for ISO 8601. Regardless, Postgres will use non-zero days when configured this way so that must be handled somehow. In my own programs I just use Span.

I said as much earlier in the thread but the original author did not respond after I pointed this out.

I don't think an implementation that doesn't address this somehow should be published personally.

Just to confirm there is a correctness issue here, this implementation omits two fields, days and months.

Okay thanks, I understand now. I agree.

I'll take a closer look later.

@maxcountryman Thanks for your feedback! Your analysis is correct.

The decode part is added three days ago at 2f17955 while I'd like to use SignedDuration to hold PG's interval. The background in my use case is that all the interval is written by my code and thus it's guaranteed only microseconds unit are filled. But for general purpose usage, this may not be true.

There is a related discussion at #174 (reply in thread). In short, I'd first remove the decode impl for SignedDuration now because you can hardly convert days & months to seconds without a relative datetime by jiff's design. But perhaps we can support codec over Span.

However, the encode part of SignedDuration should be fine.

Or instead, we may check months and days are 0 when decode to SignedDuration and return an error if so.

This reverts commit 2f17955.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2025-01-25T11:54:40Z

jiff-sqlx/src/postgres/interval.rs

@maxcountryman I'd re-add the impl for decoding SignedDuration.

I saw your reaction on the comment but unsure what the concrete issue you found. Maybe you can reply here.

For support of Span and convert between Span and PgInterval for sqlx codec, I'd leave it to the next PR if we get progress here, or there is a real usage requirement.

This PR adds a new `jiff-sqlx` crate. It defines wrapper types for `Timestamp`, `DateTime`, `Date`, `Time` and `Span`. For each wrapper type, the SQLx encoding traits are implemented. (Except, with `Span`, only the decoding trait is implemented.) This is similar to #141, but organizes things a bit differently. This also comes with SQLite support. MySQL support is missing since it seems, at present, to require exposing APIs in SQLx for a correct implementation. This initial implementation also omits `Zoned` entirely. I've left a comment in the source code explaining why. The quick summary is that, at least for PostgreSQL, I don't see a way to provide support for it without either silently losing data (the time zone) or just storing it as an RFC 9557 timestamp in a `TEXT` field. The downside of the latter is that it doesn't use PostgreSQL native datetime types. (Becuase we can't. Because PostgreSQL doesn't support storing anything other than civil time and timestamps with respect to its datetime types.) I do personally lean toward just using RFC 9557 as a `TEXT` type, but I'd like to collect real use cases first to make sure that's the right way to go. Ref #50, Closes #141 Ref launchbadge/sqlx#3487

BurntSushi · 2025-02-07T03:53:25Z

I ended up going through the exercise to do this myself. I think that was important, because it helped me learn some things I otherwise wouldn't have learned. And it also let me get confidence in the wrapper technique.

I also think that we can't do a SignedDuration impl here. It is way too fraught. We can actually barely even do Span. We can decode it just fine from PostgreSQL, but encoding one is a little trickier. I think it's possible, but it will require a little more work. I'm also somewhat worried about it because, while I think encoding is possible, it will definitely be lossy. i.e., It will lose the difference between 2 hours and 120 minutes (because all time units will need to collapse to microseconds).

maxcountryman · 2025-02-07T04:12:48Z

Agree that SignedDuration should not be used. 👍

tisonkun · 2025-02-07T05:26:31Z

I also think that we can't do a SignedDuration impl here. It is way too fraught.

Could you elaborate a bit?

he encode part would be fine and as long as we treat illegal inputs as errors, the decode part can work on the legal partial.

tisonkun · 2025-02-07T05:29:37Z

I think it's possible, but it will require a little more work. I'm also somewhat worried about it because, while I think encoding is possible, it will definitely be lossy. i.e., It will lose the difference between 2 hours and 120 minutes (because all time units will need to collapse to microseconds).

Yes. This is the background of our previous discussion at #174 (reply in thread)

tisonkun · 2025-02-07T05:30:38Z

Superseded by #240.

BurntSushi · 2025-02-07T12:19:21Z

I also think that we can't do a SignedDuration impl here. It is way too fraught.

Could you elaborate a bit?

he encode part would be fine and as long as we treat illegal inputs as errors, the decode part can work on the legal partial.

Yeah I think your current implementation doesn't have footguns per se, since it does return errors and doesn't silently drop anything. And perhaps the Encode trait implementation is warranted at least. But I'm a little iffy on the Decode side since it seems surprising that it will fail in cases with non-zero months or days.

I am open to adding this back, but I'd like to see what the use cases are first. And I note that, I believe, there aren't SQLx trait implementations for either of the duration types found in chrono and time.

This PR adds a new `jiff-sqlx` crate. It defines wrapper types for `Timestamp`, `DateTime`, `Date`, `Time` and `Span`. For each wrapper type, the SQLx encoding traits are implemented. (Except, with `Span`, only the decoding trait is implemented.) This is similar to #141, but organizes things a bit differently. This also comes with SQLite support. MySQL support is missing since it seems, at present, to require exposing APIs in SQLx for a correct implementation. This initial implementation also omits `Zoned` entirely. I've left a comment in the source code explaining why. The quick summary is that, at least for PostgreSQL, I don't see a way to provide support for it without either silently losing data (the time zone) or just storing it as an RFC 9557 timestamp in a `TEXT` field. The downside of the latter is that it doesn't use PostgreSQL native datetime types. (Becuase we can't. Because PostgreSQL doesn't support storing anything other than civil time and timestamps with respect to its datetime types.) I do personally lean toward just using RFC 9557 as a `TEXT` type, but I'd like to collect real use cases first to make sure that's the right way to go. Ref #50, Closes #141 Ref launchbadge/sqlx#3487

BurntSushi · 2025-02-07T17:24:17Z

Oh interesting, Diesel actually has an impl for chrono::Duration: https://github.com/diesel-rs/diesel/blob/edd7bd32f52a52f3f451a0fd9820fc8dcf8342a8/diesel/src/pg/types/date_and_time/chrono.rs#L170-L210

It does bad juju like assuming there are always 30 days in a month.

tisonkun added 2 commits October 11, 2024 13:10

feat: bootstrap sqlx-jiff development

20ecbd6

Signed-off-by: tison <wander4096@gmail.com>

builtin features instead

89f6330

Signed-off-by: tison <wander4096@gmail.com>

tisonkun added 2 commits October 11, 2024 13:41

Revert "builtin features instead"

309cc8d

This reverts commit 89f6330.

implement integration as a separate crate

1b248f2

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the sqlx-jiff branch from 02facb3 to 1b248f2 Compare October 11, 2024 19:46

tisonkun commented Oct 11, 2024

View reviewed changes

sqlx-jiff/src/lib.rs Outdated Show resolved Hide resolved

tisonkun commented Oct 11, 2024

View reviewed changes

sqlx-jiff/src/lib.rs Outdated Show resolved Hide resolved

tisonkun added 3 commits October 11, 2024 14:03

no pub field but to_jiff and to_sqlx

bd6bdcd

Signed-off-by: tison <wander4096@gmail.com>

sqlx-jiff to jiff-sqlx

de9e83b

Signed-off-by: tison <wander4096@gmail.com>

minimize the implementation

b6372d3

Signed-off-by: tison <wander4096@gmail.com>

tisonkun changed the title ~~feat: bootstrap sqlx-jiff development~~ feat: bootstrap jiff-sqlx development Oct 14, 2024

tisonkun added 3 commits October 23, 2024 14:16

wrap SignedDuration

4a6548f

Signed-off-by: tison <wander4096@gmail.com>

bridge Interval to SignedDuration

c3ee65b

Signed-off-by: tison <wander4096@gmail.com>

wrap civil Date and Time

d8472eb

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested a review from BurntSushi October 23, 2024 07:21

bridge Date and Time

ab4f1af

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the sqlx-jiff branch from 688129c to ab4f1af Compare October 26, 2024 17:53

paolobarbolini reviewed Nov 22, 2024

View reviewed changes

jiff-sqlx/Cargo.toml Outdated Show resolved Hide resolved

tisonkun added 4 commits November 22, 2024 22:33

Merge branch 'master' into sqlx-jiff

9c99b7c

no default sqlx features

54509f0

Signed-off-by: tison <wander4096@gmail.com>

Merge branch 'master' into sqlx-jiff

822b9e1

impl Decode for SignedDuration

2f17955

Signed-off-by: tison <wander4096@gmail.com>

Merge branch 'master' into sqlx-jiff

4513c1b

BurntSushi reviewed Jan 4, 2025

View reviewed changes

tisonkun added 4 commits January 5, 2025 08:33

Revert "impl Decode for SignedDuration"

9d4000c

This reverts commit 2f17955.

Merge branch 'master' into sqlx-jiff

c6590fe

Merge branch 'master' into sqlx-jiff

4df803f

impl Decode for SignedDuration

082b29c

Signed-off-by: tison <wander4096@gmail.com>

tisonkun commented Jan 25, 2025

View reviewed changes

BurntSushi mentioned this pull request Feb 7, 2025

jiff-sqlx: add integration crate for SQLx #240

Merged

tisonkun closed this Feb 7, 2025

tisonkun deleted the sqlx-jiff branch February 7, 2025 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bootstrap jiff-sqlx development #141

feat: bootstrap jiff-sqlx development #141

tisonkun commented Oct 11, 2024

BurntSushi commented Oct 11, 2024

tisonkun left a comment

BurntSushi commented Oct 11, 2024

tisonkun commented Oct 11, 2024 •

edited

Loading

tisonkun commented Oct 23, 2024

tisonkun commented Oct 23, 2024

maxcountryman commented Nov 13, 2024

tisonkun commented Nov 13, 2024

maxcountryman commented Nov 15, 2024

tisonkun commented Jan 4, 2025 •

edited

Loading

BurntSushi commented Jan 4, 2025

tisonkun commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi Jan 4, 2025

maxcountryman Jan 4, 2025

maxcountryman Jan 4, 2025

BurntSushi Jan 4, 2025

tisonkun Jan 5, 2025 •

edited

Loading

tisonkun Jan 5, 2025

tisonkun Jan 25, 2025

BurntSushi commented Feb 7, 2025

maxcountryman commented Feb 7, 2025

tisonkun commented Feb 7, 2025

tisonkun commented Feb 7, 2025

tisonkun commented Feb 7, 2025

BurntSushi commented Feb 7, 2025

BurntSushi commented Feb 7, 2025

feat: bootstrap jiff-sqlx development #141

feat: bootstrap jiff-sqlx development #141

Conversation

tisonkun commented Oct 11, 2024

BurntSushi commented Oct 11, 2024

tisonkun left a comment

Choose a reason for hiding this comment

BurntSushi commented Oct 11, 2024

tisonkun commented Oct 11, 2024 • edited Loading

tisonkun commented Oct 23, 2024

tisonkun commented Oct 23, 2024

maxcountryman commented Nov 13, 2024

tisonkun commented Nov 13, 2024

maxcountryman commented Nov 15, 2024

tisonkun commented Jan 4, 2025 • edited Loading

BurntSushi commented Jan 4, 2025

tisonkun commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi commented Jan 4, 2025

maxcountryman commented Jan 4, 2025

BurntSushi Jan 4, 2025

Choose a reason for hiding this comment

maxcountryman Jan 4, 2025

Choose a reason for hiding this comment

maxcountryman Jan 4, 2025

Choose a reason for hiding this comment

BurntSushi Jan 4, 2025

Choose a reason for hiding this comment

tisonkun Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

tisonkun Jan 5, 2025

Choose a reason for hiding this comment

tisonkun Jan 25, 2025

Choose a reason for hiding this comment

BurntSushi commented Feb 7, 2025

maxcountryman commented Feb 7, 2025

tisonkun commented Feb 7, 2025

tisonkun commented Feb 7, 2025

tisonkun commented Feb 7, 2025

BurntSushi commented Feb 7, 2025

BurntSushi commented Feb 7, 2025

tisonkun commented Oct 11, 2024 •

edited

Loading

tisonkun commented Jan 4, 2025 •

edited

Loading

tisonkun Jan 5, 2025 •

edited

Loading