Timestamp/Timestamptz coercion for "now - '30 days'"

[searing]

I'm having trouble expressing "now - '30 days'" in a filter expression. This short code sample illustrates what I want:

#[macro_use]
extern crate diesel;

use diesel::dsl::*;
use diesel::prelude::*;

mod schema {
    table! {
        test_table (id) {
            id -> Int8,
            foo -> Nullable<Timestamptz>,
        }
    }
}

use schema::test_table;

fn main() {
    // this is what I want to accomplish
    let foo = test_table::table
        .filter(test_table::foo.lt(diesel::dsl::sql("now() - '30 days'")));

    // this should compile
    let foo2 = test_table::table
        .filter(test_table::foo.lt((now - 30.days()).nullable()));
}

This fails to compile at "foo2" with the following error:

error[E0271]: type mismatch resolving `<diesel::expression::nullable::Nullable<diesel::expression::ops::Sub<diesel::dsl::now, diesel::expression::bound::Bound<diesel::sql_types::Interval, diesel::data_types::PgInterval>>> as diesel::Expression>::SqlType == diesel::sql_types::Nullable<diesel::sql_types::Timestamptz>`
  --> src\main.rs:25:33
   |
25 |         .filter(test_table::foo.lt((now - 30.days()).nullable()));
   |                                 ^^ expected struct `diesel::sql_types::Timestamp`, found struct `diesel::sql_types::Timestamptz`
   |
   = note: expected type `diesel::sql_types::Nullable<diesel::sql_types::Timestamp>`
              found type `diesel::sql_types::Nullable<diesel::sql_types::Timestamptz>`

Being able to get rid of the "nullable()" call would also be nice, but might be a separate issue (or difficult/impossible).

[jeremybmerrill]

@searing : Were you able to find a solution to this? I have the same problem (minus the nullable stuff), so "type mismatch resolving <diesel::expression::ops::Sub<diesel::dsl::now, diesel::expression::bound::Bound<diesel::sql_types::Interval, diesel::data_types::PgInterval>> as diesel::Expression>::SqlType == diesel::sql_types::Timestamptz" for .filter(created_at.gt( now - 1.months() ))

[searing]

No, my temporary solution is to use diesel::dsl::sql until this is fixed, hopefully in the next major Diesel release.

[jeremybmerrill]

@searing Ah okay. Thanks!

[brandur]

I'm not a maintainer, but I was looking at how this might be solved just out of curiosity. I think that the root of the problem is that the now struct is foremost fundamentally a Timestamp rather than a Timestamptz:

/// Represents the SQL `CURRENT_TIMESTAMP` constant. This is equivalent to the
/// `NOW()` function on backends that support it.
#[allow(non_camel_case_types)]
#[derive(Debug, Copy, Clone, QueryId)]
pub struct now;

impl Expression for now {
    type SqlType = Timestamp;
}

When you subtract an Interval from a Timestamp, you end up with another Timestamp, which is what's producing the error you're seeing:

impl Sub for super::Timestamp {
    type Rhs = super::Interval;
    type Output = super::Timestamp;
}

(And an off-handed comment: I found it helpful to read the obtuse error produced by the compiler backwards. When it says expected struct `diesel::sql_types::Timestamp`, found struct `diesel::sql_types::Timestamptz`, what it really means is that it wanted a Timestamptz based off your schema definition, and found Timestamp.)

Coerce is implemented for now so that it can become a Timestamptz:

#[cfg(feature = "postgres")]
impl AsExpression<Timestamptz> for now {
    type Expression = Coerce<now, Timestamptz>;

    fn as_expression(self) -> Self::Expression {
        Coerce::new(self)
    }
}

#[cfg(feature = "postgres")]
impl AsExpression<Nullable<Timestamptz>> for now {
    type Expression = Coerce<now, Nullable<Timestamptz>>;

    fn as_expression(self) -> Self::Expression {
        Coerce::new(self)
    }
}

But it doesn't seem to happen soon enough. I would think that even after the interval subtraction, it should be able to coerce the resulting Timestamp to a Timestamptz, but it apparently doesn't, and even after reading the code for some time I still don't understand how exactly coerce is supposed to work. There's a little documentation explaining what it's for, but neither it nor its implementation provide much in the way of hints as to what it's doing.

I was able to fix the problem by introducing a new struct nowtz for Postgres (and appropriate add/sub implementations):

diff --git a/diesel/src/expression/functions/date_and_time.rs b/diesel/src/expression/functions/date_and_time.rs
index 1461b8e7..d931bce9 100644
--- a/diesel/src/expression/functions/date_and_time.rs
+++ b/diesel/src/expression/functions/date_and_time.rs
@@ -46,6 +46,35 @@ let today: chrono::NaiveDate = diesel::select(date(now)).first(&connection).unwr
 ",
 "The return type of [`date(expr)`](../dsl/fn.date.html)");

+#[allow(non_camel_case_types)]
+#[cfg(feature = "postgres")]
+#[derive(Debug, Copy, Clone, QueryId)]
+pub struct nowtz;
+
+#[cfg(feature = "postgres")]
+impl Expression for nowtz {
+    type SqlType = Timestamptz;
+}
+
+#[cfg(feature = "postgres")]
+impl NonAggregate for nowtz {}
+
+#[cfg(feature = "postgres")]
+impl<DB: Backend> QueryFragment<DB> for nowtz {
+    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
+        out.push_sql("CURRENT_TIMESTAMP");
+        Ok(())
+    }
+}
+
+#[cfg(feature = "postgres")]
+impl_selectable_expression!(nowtz);
+
+#[cfg(feature = "postgres")]
+operator_allowed!(nowtz, Add, add);
+#[cfg(feature = "postgres")]
+operator_allowed!(nowtz, Sub, sub);
+
 #[cfg(feature = "postgres")]
 use expression::AsExpression;
 #[cfg(feature = "postgres")]
diff --git a/diesel/src/sql_types/ops.rs b/diesel/src/sql_types/ops.rs
index afb35d67..46f53678 100644
--- a/diesel/src/sql_types/ops.rs
+++ b/diesel/src/sql_types/ops.rs
@@ -170,3 +170,27 @@ impl Sub for super::Nullable<super::Timestamp> {
     type Rhs = super::Nullable<super::Interval>;
     type Output = super::Nullable<super::Timestamp>;
 }
+
+#[cfg(feature = "postgres")]
+impl Add for super::Timestamptz {
+    type Rhs = super::Interval;
+    type Output = super::Timestamptz;
+}
+
+#[cfg(feature = "postgres")]
+impl Add for super::Nullable<super::Timestamptz> {
+    type Rhs = super::Nullable<super::Interval>;
+    type Output = super::Nullable<super::Timestamptz>;
+}
+
+#[cfg(feature = "postgres")]
+impl Sub for super::Timestamptz {
+    type Rhs = super::Interval;
+    type Output = super::Timestamptz;
+}
+
+#[cfg(feature = "postgres")]
+impl Sub for super::Nullable<super::Timestamptz> {
+    type Rhs = super::Nullable<super::Interval>;
+    type Output = super::Nullable<super::Timestamptz>;
+}

And then changing your line above to use nowtz instead of now:

    let foo2 = test_table::table
        .filter(test_table::foo.lt((nowtz - 30.days()).nullable()))

... but I assume that there's a much more elegant way to go about this which is a little beyond my grasp. If anyone more familiar with the type system could give me a high level explanation for how this might be generally solved, I could try my hand at sending over a patch.

[searing]

I suppose you could introduce a new SQL type (perhaps Now) which can be coerced to either Timestamp or Timestamptz, and then implement interval addition/subtraction for that type. That seems like the most "correct" way to do it, in the sense that the Postgres server is probably doing something vaguely similar. I'm not sure how we'd feel about adding a new SQL type for what might be a fairly simple case, but I think it's better than nowtz since, to the user, the extra Now type should be totally transparent.

[brandur]

I suppose you could introduce a new SQL type (perhaps Now) which can be coerced to either Timestamp or Timestamptz, and then implement interval addition/subtraction for that type.

So the idea there would be that Now - Interval = Now so that you could then safely coerce to either Timestamp or Timestamptz right? I couldn't say for sure whether that would work, but it sounds like a reasonable idea. The way Diesel seems modeled today though is that SQL types are modeled after actual types in SQL, so the abstraction might still be a bit leaky.

That seems like the most "correct" way to do it, in the sense that the Postgres server is probably doing something vaguely similar.

I took a quick look at the datetime docs in Postgres and interestingly, now() is stated to unequivocally return a timestamp with time zone. It must be coerced back to a regular timestamp as needed.

I'm not sure how we'd feel about adding a new SQL type for what might be a fairly simple case, but I think it's better than nowtz since, to the user, the extra Now type should be totally transparent.

Yeah, I tried my current solution to verify that I had at least a partial understanding of the problem, but it's not very good.

[alex0ide]

Doesn't this make the query in the "Complex Queries" tab in the examples on https://diesel.rs invalid?

[weiznich]

@alex0ide Not really as it works if date in that example has the "right" sql type (timestamp, not timestamptz.)

As for the original example: The following query works and generates the corresponding sql:

    let foo2 = test_table::table
        .filter(test_table::foo.lt((now.into_sql::<Timestamptz>() - 30.days()).nullable()));

[Fuuzetsu]

I had a somewhat related issue. I wanted to use the DATE postgres function on a field that was Timestamptz. diesel is only happy with Timestamp. For anyone stumbling upon this, here's what I did:

diesel::sql_function! {
    #[sql_name="DATE"]
    fn datetz(x: diesel::sql_types::Timestamptz) -> diesel::sql_types::Timestamp
}

Now you should be able to use your datetz function instead of date and get the right types out.

[weiznich]

@Fuuzetsu That's not really this issue. It's the same underlying problem, but it can be fixed much more easily. Instead of just accepting a Timestamp type here that could be easily changed to accept an generic type that implements a trait (let's call it TimestampLike) which is implemented for Timestamp and Timestamptz. I would accept a PR for this as long as it adds an test case for this.

[Litarvan]

Still having the issue, the workaround of @.weiznich works well but it would be nice to have implicit coercion instead if possible, or an error message hinting to the solution since the current one is quite misleading imo

[weiznich]

@Litarvan As written above: We are happy to accept a PR that fixes this issues or that improves the error message. I fear the state will be as it is as long as noone is willing to put in the work to fix it. After all that's still a project run in our free time and there are features like this one that are not considered important enough to spend time fixing them.

[Litarvan]

Sorry I didn't mean this as a request, I mistakenly skipped your last comment because my use case was different than Fuuzetsu's one; I would actually love to try fixing it!

Alongside the date function improvement that you suggested, I was also wondering if it was possible for this to work:

    let foo2 = test_table::table
        .filter(test_table::foo.lt((now - 30.days()).nullable()));

i.e. not having to do .into_sql::<Timestamptz>. It would prevent users like me or @.jeremybmerrill to end up on this issue since the error is pretty hard to understand when not using nullable():

type mismatch resolving `<Sub<now, Bound<Interval, PgInterval>> as Expression>::SqlType == Timestamptz`
note: required for `diesel::expression::ops::numeric::Sub<now, diesel::expression::bound::Bound<diesel::sql_types::Interval, PgInterval>>` to implement `AsExpression<Timestamptz>` 

At first I thought it would be nice for now to be of type Timestamptz instead of Timestamp when using the Postgres backend, as the Postgres now function does return a Timestamptz actually, but this would probably break a lot of existing code so I guess it's probably not the right path.

The best thing would be for the snippet to work as-is, but I don't think it's possible: the lt function expects (omitting Nullable to simply) AsExpression<Timestamp> and the Sub expression is Expression<SqlType = Timestamptz>, and implementing AsExpression<Timestamp> for Expression<SqlType = Timestamptz> creates lot of conflicting impls, which is understandable.

So I don't see any solution other than creating another now function such as nowtz like @.brandur suggested which would probably work, but I guess it's not that much of an improvement since having a different name from the actual SQL function will probably prevent most users from knowing that the function exists, and users like me will probably keep having issues understanding the error they initially have.

At the end, I'm thinking the best solution may be to implement a custom compilation error message for this specific case like diesel recently did for select mismatches, I don't know if it's possible but I could maybe try that :thinking:

[weiznich]

We already have a AsExpression<Timestamptz> impl for now here: https://github.com/diesel-rs/diesel/blob/f7e9819303877b5a255143c4b32fa1e5745aaad0/diesel/src/expression/functions/date_and_time.rs#L62

Unfortunately that doesn't really help in this particular case as we don't implement the std::ops::Sub trait for E: AsExpression<Timestamptz> but only for expressions of the same type as we otherwise run into type inference issues due to ambiguities. (Relevant code is here: https://github.com/diesel-rs/diesel/blob/f7e9819303877b5a255143c4b32fa1e5745aaad0/diesel/src/expression/ops/mod.rs#L3)

A custom compilation error would be nice but I fear that will require changes to the rust compiler. The basic infrastructure for this is already there via the #[diagnostic] attribute namespace, but it would still require an additional attribute that allows to customize error messages for type mismatches. As far as I know the compiler team is open to such an attribute, it just needs someone that spends the time implementing it. I even can provide some pointers how to do that if someone is interested.