npgsql/EntityFramework6.Npgsql

[Bug] Incorrect association of "time" with DbType "interval" in SqlBaseGenerator.cs

Arsync opened this issue · 21 comments

When we have a column with dbtype "time",
for example, coulmn "my_time time", and auto-generated code be the same as

SELECT case when ... then CAST(NULL as interval) else ("Project1"."my_time") end

then got exception like
"In CASE construction cannot generalize interval and time without timezone types".

Error source is here: SqlBaseGenerator, line 790:

case PrimitiveTypeKind.Time:
  return "interval";
roji commented

@Arsync what do you think it should be mapped to? time without timezone would produce the same error...

When column at the table has "time" type, it must be "time" for case, when we need to get NULL for that column. In raw sql I've solve same things by using NULL::time. Just an issue where our team is stuck while migrating EF6 from SQL Server to Postgres.

Emill commented

What use case do you have for using the "time" type in the first place?

So, "time of born" is not interval... It just time column.

roji commented

It's true that at the ADO.NET level, Npgsql maps DbType.Time to PostgreSQL time without time stamp. However, making this kind of change now could break a lot of people. Also, the EF6 provider is more or less in bugfix mode - no big changes are done to it unless necessary.

Do you have any actual, concrete problem with mapping to interval? As I mentioned above, changing to PG time wouldn't make the CASE-related error message above go away.

We've got this one in usual .Include operation:

CASE WHEN (NOT ("Project1"."C1" = TRUE AND "Project1"."C1" IS NOT NULL))
THEN (CAST(NULL AS inerval))
WHEN ("Project1"."C9" = TRUE AND "Project1"."C9" IS NOT NULL)
THEN ("Project1"."time_of_born") END AS "C32"

It breaks select operation with exception, nothing special was doing...

roji commented

Once again, you said that the error you're seeing is In CASE construction cannot generalize interval and time without timezone types. That means that even if we change from interval to time, you'd still get the same error.

Sorry for my English. Run this code to see the exact problem in SqlGenerator:

CREATE TABLE pg_temp.abc (

    time_of_born time
);

-- Error
SELECT
    case when TRUE then
        CAST(NULL as interval)
    else time_of_born end   
FROM pg_temp.abc;

-- Success
SELECT
    case when TRUE then
        CAST(NULL as time)
    else time_of_born end   
FROM pg_temp.abc;
roji commented

That's because your table is already defined with time - did you create it manually yourself?

Doing this against an interval column seems to work fine:

CREATE TABLE with_time (time_of_born TIME);

SELECT
    case when TRUE then
             CAST(NULL as TIME)
         else time_of_born end
FROM with_time;

In our entity mappings we using .HasColumnType("time") for time columns. And it breaks when including TPT entities with this column.

So we have table A and table B.
B has 'time_of_born time'.
When we doing A.Include(B), it throws the exception as seen above...

If one of subclasses of B has this property and another has not. So it time for one and NULL for another. But second one tries to cast it to interval type internally in SqlBaseGenerator.

roji commented

Have you tried doing HasColumnType("time") HasColumnType("interval") instead?

It breaks exactly with this mapping. Without it TimeSpan type mapped to interval type of column, which is not applicable to our business process.

roji commented

Sorry, I meant to ask if you tried doing HasColumnType("interval") above, not time.

interval is what the EF6 provider maps to time - this is just how it is. It's not surprising that when you try to force the mapping to a different type of column things go bad... Other users may currently depend on mapping PrimitiveTypeKind.Time to interval, and would break in the same way if we changed it. I do agree that if I were building the provider from scratch today, I'd probably map to time - but that's a very different question./

Any specific interval isn't applicable for your business process?

The time type is more native for our case. I will think about interval, but looking for option, that can enable time instead of interval - like NpgsqlConnection.GlobalTypeMapper for NodaTime. But SqlBaseGenerator looks into local method instead.

We moving to EF Core in the nearest future, but several month goes with EF6 support.

So when child was born: time '01:03' or interval '1 hours 3 minutes'? :)
In this case, interval of what? And there is many other things like that.

Interval is applicable when reflects some time distance. And time column is a point at this distance. We keep exactly points in time.

roji commented

@Arsync that's largely a semantic difference, you can ask "time from what, from when" on time just as well as from interval. You can use interval and use it to mean "from the start of the day".

In my opinion, an interval is always some result of calculation operations. It is not point to keep in a table column. But yes, it's semantic difference. But becomes more difficult when time zone is coming to the scene. So keep time column in one table and time with timezone in another is more uniform, than interval and time with timezone.

roji commented

In my opinion, an interval is always some result of calculation operations. It is not point to keep in a table column

That really is just an opinion - interval is a 1st-class PG data type exactly like time, and I'm aware of some projects which store interval column.

But becomes more difficult when time zone is coming to the scene. So keep time column in one table and time with timezone in another is more uniform, than interval and time with timezone.

In PG, time with time zone is a completely different type from time - which does not have a time zone. If we mapped EF6's PrimitiveTypeKind.Time to time, you would have exactly the same issue with it not having a time zone. And it would not make any sense to map to time with time zone, since TimeSpan does not have a timezone.

My advice is to carefully think why interval really is unsuitable for you - EF6 has its current mapping, which is to interval, and you're trying to work against that.

Now I found one of use cases where time is useful: as native range restriction.
Time as a part of day reflects 24-hours, while intervals is unlimited in its range. So baby may born
2020-05-12 at 27 hours 5 minutes... or even 2 days of interval at same day.
Anyway, if there is any column with value out of range, we can get exception.

And even more - interval can be negative, which is not applicable to time.
I know, we can use CHECK constraint, but if there many columns to check, it may overhead maintenance.

Emill commented

Why don't you store the born time as a full "timestamp with time zone", which is a UTC timestamp that includes both date and time?

Good point, but it related to our lead developer and quite an ancient project in production, you know. So, sometimes this 'time' type is useful. If this point can be reached at EF Core, it will be good. Now I'm trying to migrate for 'interval' type.

roji commented

Am going to go ahead and close this, as I don't think we'll be changing the provider mapping - it would be too big of a breaking change.