FCO / Red

A WiP ORM for Raku
Artistic License 2.0
70 stars 27 forks source link

Regression in DateTime comparison operators on Pg #570

Closed jonathanstowe closed 8 months ago

jonathanstowe commented 8 months ago

Hi, Given a DateTime column like

has DateTime    $.created       is column;

And a method on the model:

    method clear( --> Nil ) {
        self.^all.grep( *.created < DateTime.now.earlier(days => 1)).delete;
    }

Previously would have generated the SQL:

SQL : DELETE FROM user_session
WHERE "user_session".created < '2024-02-19T07:41:25.179502Z'
BIND: []

Whereas with the current HEAD it gives:

SQL : DELETE FROM user_session
WHERE ("user_session".created)::NUMERIC < '2024-02-19T07:53:15.313644Z'
RETURNING *
BIND: []
    Unknown Error!!!
    Please, copy this backtrace and open an issue on https://github.com/FCO/Red/issues/new
    Driver: Red::Driver::Pg
    Original error: DB::Pg::Error::FatalError.new(message => "invalid input syntax for type numeric: \"2024-02-19T07:53:15.313644Z\"", message-detail => Str, message-hint => Str, context => Str, type => "ERROR", type-localized => "ERROR", state => "22P02", statement-position => "68", internal-position => Str, internal-query => Str, schema => Str, table => Str, column => Str, datatype => Str, constraint => Str, source-file => "numeric.c", source-line => "621", source-function => "numeric_in")

So it would appear that the unnecessary cast was introduced at some point as a side effect of some other change. I'll try and bisect this at some point.

jonathanstowe commented 8 months ago

It looks like it was introduced in the b0ce7c872a79afde7c9509d4d15ee751b19a7def :

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[b0ce7c872a79afde7c9509d4d15ee751b19a7def] Revert "Revert "Make Red::Type be able to add methods on column""

Which seems plausible as it does amend the way the cast is applied.

Tested with

#!/usr/bin/env raku

use Red;
use Test;

my $*RED-DB = database 'Pg', dbname => 'res_test';
my $*RED-DEBUG = True;

model Dt {
    has Str $.id is id;
    has DateTime $.created is column = DateTime.now;

    method clear( --> Nil ) {
        self.^all.grep( *.created < DateTime.now.earlier(days => 1)).delete;
    }

}

# Dt.^create-table;

lives-ok { Dt.clear };

# vim: ft=raku
jonathanstowe commented 8 months ago

Just for completeness, if the operator is changed to lt then it emits:

SQL : DELETE FROM dt
WHERE ("dt".created)::TEXT < '2024-02-19T18:41:46.280906Z'
RETURNING *
BIND: []
ok 1 - 

Which doesn't lead to a failure, but it is still probably wrong because Pg will actually cast the RHS implicitly to a datetime-ish thing if the LHS is a datetime-ish thing.

FCO commented 8 months ago

I'll make it cast for date time... sorry for not seeing that before. I'll try to fix that tomorrow

FCO commented 8 months ago

It seems it was a copy-pasto... thanks! (sorry for that)

jonathanstowe commented 8 months ago

I still don't think that is right.

Given the (non exhaustive,) tests:

#!/usr/bin/env raku

use Red;
use Test;

my $*RED-DB = database 'Pg', dbname => 'res_test';
my $*RED-DEBUG = True;

model Dt {
    has Int $.id is serial;
    has DateTime $.created is column = DateTime.now;

}

Dt.^create-table(:if-not-exists);

Dt.^all.delete;

my $earlier = DateTime.now.earlier(days => 1);
my $even_earlier = DateTime.now.earlier(days => 2);

Dt.^create(created => $earlier);

my $now = DateTime.now;

lives-ok { is Dt.^all.grep(*.created < $now).elems, 1, "$earlier < $now" };
lives-ok { is Dt.^all.grep(*.created lt $now).elems, 1, "$earlier lt $now" };
lives-ok { is Dt.^all.grep(*.created < $earlier).elems, 0, "$earlier < $earlier" };
lives-ok { is Dt.^all.grep(*.created lt $earlier).elems, 0, "$earlier lt $earlier" };
lives-ok { is Dt.^all.grep(*.created <= $now).elems, 1, "$earlier <= $now" };
lives-ok { is Dt.^all.grep(*.created le $now).elems, 1, "$earlier le $now" };
lives-ok { is Dt.^all.grep(*.created <= $earlier).elems, 1, "$earlier <= $earlier" };
lives-ok { is Dt.^all.grep(*.created le $earlier).elems, 1, "$earlier le $earlier" };
lives-ok { is Dt.^all.grep(*.created <= $even_earlier).elems, 0, "$earlier <= $even_earlier" };
lives-ok { is Dt.^all.grep(*.created le $even_earlier).elems, 0, "$earlier le $even_earlier" };
lives-ok { is Dt.^all.grep(*.created == $now).elems, 0, "$earlier == $now" };
lives-ok { is Dt.^all.grep(*.created eq $now).elems, 0, "$earlier eq $now" };
lives-ok { is Dt.^all.grep(*.created == $earlier).elems, 1, "$earlier == $earlier" };
lives-ok { is Dt.^all.grep(*.created eq $earlier).elems, 1, "$earlier eq $earlier" };
lives-ok { is Dt.^all.grep(*.created != $earlier).elems, 0, "$earlier != $earlier" };
lives-ok { is Dt.^all.grep(*.created ne $earlier).elems, 0, "$earlier ne $earlier" };
lives-ok { is Dt.^all.grep(*.created != $now).elems, 1, "$earlier != $now" };
lives-ok { is Dt.^all.grep(*.created ne $now).elems, 1, "$earlier ne $now" };
lives-ok { is Dt.^all.grep(*.created > $now).elems, 0, "$earlier > $now" };
lives-ok { is Dt.^all.grep(*.created gt $now).elems, 0, "$earlier gt $now" };
lives-ok { is Dt.^all.grep(*.created > $even_earlier).elems, 1, "$earlier > $even_earlier" };
lives-ok { is Dt.^all.grep(*.created gt $even_earlier).elems, 1, "$earlier gt $even_earlier" };
lives-ok { is Dt.^all.grep(*.created >= $even_earlier).elems, 1, "$earlier >= $even_earlier" };
lives-ok { is Dt.^all.grep(*.created ge $even_earlier).elems, 1, "$earlier ge $even_earlier" };
lives-ok { is Dt.^all.grep(*.created >= $earlier).elems, 1, "$earlier >= $earlier" };
lives-ok { is Dt.^all.grep(*.created ge $earlier).elems, 1, "$earlier ge $earlier" };
lives-ok { is Dt.^all.grep(*.created >= $now).elems, 0, "$earlier >= $now" };
lives-ok { is Dt.^all.grep(*.created ge $now).elems, 0, "$earlier ge $now" };
# vim: ft=raku

We get

SQL : CREATE TABLE "dt" (
   id serial NOT NULL primary key,
   created timestamp NOT NULL 
)
BIND: []
SQL : DELETE FROM dt
RETURNING *
BIND: []
SQL : INSERT INTO "dt"(
   created
)
VALUES(
   $1
) RETURNING *
BIND: ["2024-02-24T13:26:56.283431Z"]
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP < $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 1 - 2024-02-24T13:26:56.283431Z < 2024-02-25T13:26:56.311482Z
ok 2 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT < $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 3 - 2024-02-24T13:26:56.283431Z lt 2024-02-25T13:26:56.311482Z
ok 4 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP < $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
ok 5 - 2024-02-24T13:26:56.283431Z < 2024-02-24T13:26:56.283431Z
ok 6 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT < $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 7 - 2024-02-24T13:26:56.283431Z lt 2024-02-24T13:26:56.283431Z
# Failed test '2024-02-24T13:26:56.283431Z lt 2024-02-24T13:26:56.283431Z'
# at ct line 30
# expected: '0'
#      got: '1'
ok 8 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP <= $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 9 - 2024-02-24T13:26:56.283431Z <= 2024-02-25T13:26:56.311482Z
ok 10 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT <= $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 11 - 2024-02-24T13:26:56.283431Z le 2024-02-25T13:26:56.311482Z
ok 12 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP <= $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
ok 13 - 2024-02-24T13:26:56.283431Z <= 2024-02-24T13:26:56.283431Z
ok 14 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT <= $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
ok 15 - 2024-02-24T13:26:56.283431Z le 2024-02-24T13:26:56.283431Z
ok 16 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP <= $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 17 - 2024-02-24T13:26:56.283431Z <= 2024-02-23T13:26:56.284559Z
ok 18 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT <= $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 19 - 2024-02-24T13:26:56.283431Z le 2024-02-23T13:26:56.284559Z
ok 20 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::NUMERIC = $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
not ok 21 - 
# Failed test at ct line 37
#     Unknown Error!!!
#     Please, copy this backtrace and open an issue on https://github.com/FCO/Red/issues/new
#     Driver: Red::Driver::Pg
#     Original error: DB::Pg::Error::FatalError.new(message => "cannot cast type timestamp without time zone to numeric", message-detail => Str, message-hint => Str, context => Str, type => "ERROR", type-localized => "ERROR", state => "42846", statement-position => "70", internal-position => Str, internal-query => Str, schema => Str, table => Str, column => Str, datatype => Str, constraint => Str, source-file => "parse_expr.c", source-line => "2748", source-function => "transformTypeCast")
# 
# Original error:
# cannot cast type timestamp without time zone to numeric
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT = $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 22 - 2024-02-24T13:26:56.283431Z eq 2024-02-25T13:26:56.311482Z
ok 23 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::NUMERIC = $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 24 - 
# Failed test at ct line 39
#     Unknown Error!!!
#     Please, copy this backtrace and open an issue on https://github.com/FCO/Red/issues/new
#     Driver: Red::Driver::Pg
#     Original error: DB::Pg::Error::FatalError.new(message => "cannot cast type timestamp without time zone to numeric", message-detail => Str, message-hint => Str, context => Str, type => "ERROR", type-localized => "ERROR", state => "42846", statement-position => "70", internal-position => Str, internal-query => Str, schema => Str, table => Str, column => Str, datatype => Str, constraint => Str, source-file => "parse_expr.c", source-line => "2748", source-function => "transformTypeCast")
# 
# Original error:
# cannot cast type timestamp without time zone to numeric
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT = $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 25 - 2024-02-24T13:26:56.283431Z eq 2024-02-24T13:26:56.283431Z
# Failed test '2024-02-24T13:26:56.283431Z eq 2024-02-24T13:26:56.283431Z'
# at ct line 40
# expected: '1'
#      got: '0'
ok 26 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::NUMERIC != $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 27 - 
# Failed test at ct line 41
#     Unknown Error!!!
#     Please, copy this backtrace and open an issue on https://github.com/FCO/Red/issues/new
#     Driver: Red::Driver::Pg
#     Original error: DB::Pg::Error::FatalError.new(message => "cannot cast type timestamp without time zone to numeric", message-detail => Str, message-hint => Str, context => Str, type => "ERROR", type-localized => "ERROR", state => "42846", statement-position => "70", internal-position => Str, internal-query => Str, schema => Str, table => Str, column => Str, datatype => Str, constraint => Str, source-file => "parse_expr.c", source-line => "2748", source-function => "transformTypeCast")
# 
# Original error:
# cannot cast type timestamp without time zone to numeric
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT != $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 28 - 2024-02-24T13:26:56.283431Z ne 2024-02-24T13:26:56.283431Z
# Failed test '2024-02-24T13:26:56.283431Z ne 2024-02-24T13:26:56.283431Z'
# at ct line 42
# expected: '0'
#      got: '1'
ok 29 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::NUMERIC != $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
not ok 30 - 
# Failed test at ct line 43
#     Unknown Error!!!
#     Please, copy this backtrace and open an issue on https://github.com/FCO/Red/issues/new
#     Driver: Red::Driver::Pg
#     Original error: DB::Pg::Error::FatalError.new(message => "cannot cast type timestamp without time zone to numeric", message-detail => Str, message-hint => Str, context => Str, type => "ERROR", type-localized => "ERROR", state => "42846", statement-position => "70", internal-position => Str, internal-query => Str, schema => Str, table => Str, column => Str, datatype => Str, constraint => Str, source-file => "parse_expr.c", source-line => "2748", source-function => "transformTypeCast")
# 
# Original error:
# cannot cast type timestamp without time zone to numeric
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT != $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 31 - 2024-02-24T13:26:56.283431Z ne 2024-02-25T13:26:56.311482Z
ok 32 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP > $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 33 - 2024-02-24T13:26:56.283431Z > 2024-02-25T13:26:56.311482Z
ok 34 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT > $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 35 - 2024-02-24T13:26:56.283431Z gt 2024-02-25T13:26:56.311482Z
ok 36 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP > $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 37 - 2024-02-24T13:26:56.283431Z > 2024-02-23T13:26:56.284559Z
ok 38 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT > $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 39 - 2024-02-24T13:26:56.283431Z gt 2024-02-23T13:26:56.284559Z
ok 40 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP >= $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 41 - 2024-02-24T13:26:56.283431Z >= 2024-02-23T13:26:56.284559Z
ok 42 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT >= $1
LIMIT 1
BIND: ["2024-02-23T13:26:56.284559Z"]
ok 43 - 2024-02-24T13:26:56.283431Z ge 2024-02-23T13:26:56.284559Z
ok 44 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP >= $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
ok 45 - 2024-02-24T13:26:56.283431Z >= 2024-02-24T13:26:56.283431Z
ok 46 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT >= $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 47 - 2024-02-24T13:26:56.283431Z ge 2024-02-24T13:26:56.283431Z
# Failed test '2024-02-24T13:26:56.283431Z ge 2024-02-24T13:26:56.283431Z'
# at ct line 52
# expected: '1'
#      got: '0'
ok 48 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TIMESTAMP >= $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 49 - 2024-02-24T13:26:56.283431Z >= 2024-02-25T13:26:56.311482Z
ok 50 - 
SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT >= $1
LIMIT 1
BIND: ["2024-02-25T13:26:56.311482Z"]
ok 51 - 2024-02-24T13:26:56.283431Z ge 2024-02-25T13:26:56.311482Z
ok 52 - 

There remains the problematic ::NUMERIC cast for the numeric equality (=, >=, <=, ) and also the ::TEXT cast for other string operators, eq.

SQL : SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT < $1
LIMIT 1
BIND: ["2024-02-24T13:26:56.283431Z"]
not ok 7 - 2024-02-24T13:26:56.283431Z lt 2024-02-24T13:26:56.283431Z
# Failed test '2024-02-24T13:26:56.283431Z lt 2024-02-24T13:26:56.283431Z'
# at ct line 30
# expected: '0'
#      got: '1'

Whereas in psql:

res_test=> SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   ("dt".created)::TEXT < '2024-02-24T13:26:56.283431Z';
 data_1 
--------
      1
(1 row)

res_test=> SELECT
   count('*') as "data_1"
FROM
   "dt"
WHERE
   "dt".created < '2024-02-24T13:26:56.283431Z';
 data_1 
--------
      0
(1 row)

It works without the cast and doesn't without it. The reason for this is that the ::TEXT representation of the timestamp differs from the string we are passing:

res_test=> select (dt.created)::TEXT from dt;
          created           
----------------------------
 2024-02-24 13:26:56.283431
(1 row)

The Pg will happily cast the string we have on the LHS:

res_test=> select '2024-02-24T13:26:56.283431Z'::TIMESTAMP from dt;
         timestamp          
----------------------------
 2024-02-24 13:26:56.283431
(1 row)

But it isn't necessary as Pg will do the right thing and cast a string on the RHS that it can parse as a timestamp implicitly, blowing up if the value can't be parsed:

SELECT                            
   count('*') as "data_1"
FROM
   "dt"
WHERE
   "dt".created < 'gghhghhg';
ERROR:  invalid input syntax for type timestamp: "gghhghhg"
LINE 6:    "dt".created < 'gghhghhg';
                          ^

My suggestion would be to not attempt to cast in SQL at all if the RHS in raku space is a DateTime, and attempt a conversion in raku space if the RHS is a string (so the error comes from raku space.)

The broader suggestion might be that the drivers themselves might have the list of the LHS => RHS pairs where the coercion is or isn't necessary.

FCO commented 8 months ago

Thanks again! I've used you code as test now... Could you confirm if that's working now, please?

jonathanstowe commented 8 months ago

Yep that looks good now :+1:

Thank you.