[WIP] Timespan support #3994

jogibear9988 · 2023-02-23T09:41:22Z

Open Questions/Tasks

Should we change the default type in Mapping Schema for TimeSpan to int64. I've done it, but is this okay?
Can I access the Column Type from my builder? Cause I want also to support for example the usage of a Bigint Column in Postgres. I've done it now like this way: var dt = ((SqlField)((SqlExpression)timeSpan).Parameters[0]).ColumnDescriptor.DataType; but sure this is not the correct way

viceroypenguin · 2023-10-15T20:54:44Z

Source/LinqToDB/Linq/Builder/ExpressionBuilder.SqlBuilder.cs

@@ -501,7 +501,7 @@ private TransformInfo ConvertExpressionTransformer(Expression e)
 						return new TransformInfo(ConvertExpression(expr));
 					}

-					if (ma.Member.DeclaringType == typeof(TimeSpan))
+					/*if (ma.Member.DeclaringType == typeof(TimeSpan))


I'm not sure I'm comfortable with take 55 lines of code and simply commenting them out. If these lines are not needed, they should be removed entirely. The whole point of source control is that the lines are still available in history if they are needed for reverting.

It's not yet finished. I will remove them if it is. I only need answers to my questions, the whole pull still needs work, and I will remove the commented out code.

I only now recoginzed, that a few others things break

viceroypenguin · 2023-10-15T20:58:40Z

Source/LinqToDB/Linq/Builder/ExpressionBuilder.SqlBuilder.cs

+						&& Nullable.GetUnderlyingType(ex.Right.Type) == typeof(TimeSpan))
+					{
+						var method = MemberHelper.MethodOf(
+												() => Sql.DateAdd(Sql.DateParts.Millisecond, 0, DateTime.MinValue));


spacing is off here. this should only be one-tab to the right of var, not six.

I will look after the formating if everything works and is okay

viceroypenguin · 2023-10-15T20:59:50Z

Source/LinqToDB/Mapping/MappingSchema.cs

@@ -1330,7 +1330,7 @@ public DefaultMappingSchema() : base(new DefaultMappingSchemaInfo())
 				AddScalarType(typeof(decimal),         DataType.Decimal);
 				AddScalarType(typeof(DateTime),        DataType.DateTime2);
 				AddScalarType(typeof(DateTimeOffset),  DataType.DateTimeOffset);
-				AddScalarType(typeof(TimeSpan),        DataType.Time);
+				AddScalarType(typeof(TimeSpan),        DataType.Int64);


This is likely to break existing consumers. It should be done as a separate PR with support from the entire team, and it would not have my support.

Don't know what will break. But clearly the Datatype "Time" wich is for a Time in Day is not the correct one

You may not like it, but historically, TimeSpan was used for the sql time type, so it is accurate to the historical and technical reality.

Yeah, for sure if we won't do this change, I can remove. But atm. I'll leave it. But whole calculation with timespans will not work with type, so everyone who will the correct functionality need than to apply a custom mapping.

There are two sides to this coin.

If you think "what should TimeSpan map to", then you're right time is a very bad choice.
If you think "what should time map to", then some people thought TimeSpan was the most fitting type in early .net.

In modern .net time would have a natural fit as TimeOnly.
For TimeSpan, SQL:2008 defines an INTERVAL type that is supported by many RDBMS including Oracle, MySQL, Postgre, MariaDB, Ingres. As such this would be a better default mapping (that would be overriden by providers not supporting INTERVAL, such as MSSQL).

In any case we need to be careful with backward compatibility. Isn't changing this default mapping is going to break existing consumers that have time in their DB?

jogibear9988 · 2023-10-15T22:22:40Z

Now a little bit more info:

At first, the pull request is not yet done, the code needs to be cleaned up (remove commented, fix spaceing, ...)
Also I now only tried mssql, oracle and postgres, the other providers will follow if the code is okay, in the way it works)

The complexity results in, that the TimeSpan datatype has no coresponding type in most databases, so the Ticks will be used (100 nanoseconds) in a bigint field. But in postgres we have a interval type, wich is used for timespan. So if I calculate a datediff automaticaly it should also be a interval type, for this I added the "DatePart.Interval" to the datediff function.
If the database has no interval type, it will return the Ticks.

jogibear9988 · 2023-10-16T11:56:00Z

Source/LinqToDB/Sql/Sql.TimeSpan.cs

+				var timeSpan = builder.GetExpression("timeSpan");
+
+				var dt = DataType.Undefined;
+				if (timeSpan is SqlField f)


Is this the correct way to check for the type of the Field in the Database?

use timeSpan.GetExpressionType() helper

jogibear9988 · 2023-10-17T19:59:01Z

/azp run test-all

Shane32 · 2023-10-17T20:38:39Z

Should we change the default type in Mapping Schema for TimeSpan to int64. I've done it, but is this okay?

I'd agree with @viceroypenguin here: historically, TimeSpan has mapped to time, and could significantly affect existing codebases. Now with .NET 6's introduction of the TimeOnly data type, it would make more sense to map TimeOnly to time and TimeSpan to bigint. But I would not change the mapping at this time, as this would be a "silent" break in the code: it could potentially cause old code to compile against the new version with no design-time notifications, but yet break production queries.

If everyone agreed that mapping TimeOnly to time and TimeSpan to bigint would be a better default mapping long-term, then I might consider removing the default mapping of TimeSpan altogether for a couple major versions, and then reintroducing it linked to bigint. In this way, users would hopefully upgrade, find that they must set the mapping manually (via global option or whatever), and later on when they upgrade again, the mapping will already be set in their code to use time rather than bigint, so the new default mapping will not break their code.

Another possibility would be to add a deprecation notice (perhaps via included .NET analyzer) to inform users that the TimeSpan mapping will change in the future, recommending users to switch to TimeOnly.

Another point of consideration is 'what is SqlClient and Entity Framework's default mapping for TimeSpan?' If the rest of the .NET community assumes that TimeSpan gets mapped to time, then it's probably best that we at linq2db keep the same default, and use some type of runtime option or mapping configuration to select bigint, even if that makes more sense in 2023 than when the default was designed in 2008 for EF 3.5.

jogibear9988 · 2023-10-18T02:54:55Z

At the moment I've switched back to "time" cause some tests break.
My Problem is, that I cannot map Timespan differently for some providers then.
In my test, I now mapped it to bigint, but for postgres I like to map it to Interval. This is only possible with this ugly code now:
https://github.com/linq2db/linq2db/pull/3994/files#diff-71d51f038daa00c195480c91854cacefd7ece6c3e109b5b48d7c99fb3d9e77adR37

If we change it, for sure it this should be a major version change. But for now it's also okay for me if we leave it.

jods4 · 2023-10-18T07:56:29Z

Can we not put the change behind an option? That's what we usually do for this kind of breaking change.

jogibear9988 · 2023-10-18T16:51:37Z

we could. atm, i'm looking that all unit tests work again. found some wich don't and then I'll look.
Where are those options normaly?

jogibear9988 · 2023-10-18T16:53:25Z

And what is for example with the expressions in the expressions.cs, they are in a static field, so how should we make them optional?
https://github.com/linq2db/linq2db/pull/3994/files#diff-2a6a9163861fbdc4015c6f32a3373c85469cb401552aea366b2d2c07ede63fb5

… already uses a mapping to interval type

jogibear9988 · 2024-04-19T12:50:27Z

/azp run test-all

azure-pipelines · 2024-04-19T12:50:40Z

Azure Pipelines successfully started running 1 pipeline(s).

linq2dbot · 2024-04-19T16:12:17Z

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

jogibear9988 · 2024-04-19T19:02:46Z

/azp run test-all

azure-pipelines · 2024-04-19T19:02:58Z

Azure Pipelines successfully started running 1 pipeline(s).

linq2dbot · 2024-04-19T21:57:12Z

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

jogibear9988 linked an issue Feb 23, 2023 that may be closed by this pull request

Problem with Calculated DateTime and Where Filter #3993

Open

MaceWindu added this to the 5.? milestone Feb 23, 2023

jogibear9988 force-pushed the issue/3993 branch from d4b14e3 to 025ab02 Compare October 14, 2023 08:54

jogibear9988 requested review from igor-tkachev, sdanyliv, MaceWindu, viceroypenguin, jods4 and Shane32 as code owners October 14, 2023 08:54

jogibear9988 changed the title ~~test for #3993~~ test for and fix for #3993 - timepsan support Oct 14, 2023

jogibear9988 changed the title ~~test for and fix for #3993 - timepsan support~~ test for and fix for #3993 - timespan support Oct 14, 2023

viceroypenguin reviewed Oct 15, 2023

View reviewed changes

linq2dbot mentioned this pull request Oct 16, 2023

Baselines for https://github.com/linq2db/linq2db/pull/3994 linq2db/linq2db.baselines#1069

Closed

jogibear9988 commented Oct 16, 2023

View reviewed changes

jogibear9988 mentioned this pull request Oct 23, 2023

Propertyaccess get's completly removed from "TimeSpan" in SQL, so wrong query created #4308

Open

linq2db deleted a comment from azure-pipelines bot Oct 23, 2023

linq2db deleted a comment from linq2dbot Oct 23, 2023

linq2db deleted a comment from azure-pipelines bot Oct 23, 2023

jogibear9988 added 21 commits April 19, 2024 08:08

possibility to create custom replacements for unary expressions

c7f1840

Fixes for Sqlite and SQL Server

b58a11e

fix mariadb/mysql -> they don't have milliseconds

15c317a

enable all databases, so i could see wich will fail

761de75

work on more databases

0cc27de

work on other data providers

5555f71

fix wrong test

7cdc747

bring back configuration setting, or postgres will not work, since it…

0285830

… already uses a mapping to interval type

fix firebird 2.5

887b0d5

fix sap hana

d9a44ed

sample of splitting up of intervals

749c8a8

refactoring

366d424

informix&sybase fix

849a4cd

sap hana

98c126d

fix for clickhouse

9198875

fix informix

f62adaf

try to fix informix

fd1fb0b

try fix clickhouse

440c264

try fix clickhouse

5cd46fe

fix informix

edfaf4c

fix clickhouse

26b42db

jogibear9988 force-pushed the issue/3993 branch from 3a8381e to 26b42db Compare April 19, 2024 06:17

linq2dbot mentioned this pull request Apr 19, 2024

Baselines for https://github.com/linq2db/linq2db/pull/3994 linq2db/linq2db.baselines#1178

Closed

linq2dbot mentioned this pull request Apr 19, 2024

Baselines for https://github.com/linq2db/linq2db/pull/3994 linq2db/linq2db.baselines#1179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Timespan support #3994

[WIP] Timespan support #3994

jogibear9988 commented Feb 23, 2023 •

edited by MaceWindu

viceroypenguin Oct 15, 2023

jogibear9988 Oct 15, 2023

viceroypenguin Oct 15, 2023

jogibear9988 Oct 15, 2023

viceroypenguin Oct 15, 2023

jogibear9988 Oct 15, 2023

viceroypenguin Oct 15, 2023

jogibear9988 Oct 15, 2023

jods4 Jan 20, 2024

jogibear9988 commented Oct 15, 2023

jogibear9988 Oct 16, 2023

MaceWindu Feb 3, 2024

jogibear9988 commented Oct 17, 2023

Shane32 commented Oct 17, 2023

jogibear9988 commented Oct 18, 2023

jods4 commented Oct 18, 2023

jogibear9988 commented Oct 18, 2023

jogibear9988 commented Oct 18, 2023 •

edited

jogibear9988 commented Apr 19, 2024

azure-pipelines bot commented Apr 19, 2024

linq2dbot commented Apr 19, 2024

jogibear9988 commented Apr 19, 2024

azure-pipelines bot commented Apr 19, 2024

linq2dbot commented Apr 19, 2024

[WIP] Timespan support #3994

Are you sure you want to change the base?

[WIP] Timespan support #3994

Conversation

jogibear9988 commented Feb 23, 2023 • edited by MaceWindu

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogibear9988 commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogibear9988 commented Oct 17, 2023

Shane32 commented Oct 17, 2023

jogibear9988 commented Oct 18, 2023

jods4 commented Oct 18, 2023

jogibear9988 commented Oct 18, 2023

jogibear9988 commented Oct 18, 2023 • edited

jogibear9988 commented Apr 19, 2024

azure-pipelines bot commented Apr 19, 2024

linq2dbot commented Apr 19, 2024

jogibear9988 commented Apr 19, 2024

azure-pipelines bot commented Apr 19, 2024

linq2dbot commented Apr 19, 2024

jogibear9988 commented Feb 23, 2023 •

edited by MaceWindu

jogibear9988 commented Oct 18, 2023 •

edited