From c484080baff333cf9605e09f243b8c49624ad610 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 28 Oct 2024 08:42:50 +0100 Subject: [PATCH] Translations around DateOnly.DayNumber (#3333) Closes #3194 --- .../NpgsqlDateTimeMemberTranslator.cs | 24 ++++++--- .../NpgsqlDateTimeMethodTranslator.cs | 16 ++++++ .../NpgsqlSqlTranslatingExpressionVisitor.cs | 42 +++++++++++----- .../Query/NpgsqlSqlExpressionFactory.cs | 12 +++++ .../Query/TimestampQueryTest.cs | 50 +++++++++++++++++++ 5 files changed, 125 insertions(+), 19 deletions(-) diff --git a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMemberTranslator.cs b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMemberTranslator.cs index 2cbac35b8..f3a44238f 100644 --- a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMemberTranslator.cs +++ b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMemberTranslator.cs @@ -56,6 +56,11 @@ public NpgsqlDateTimeMemberTranslator(IRelationalTypeMappingSource typeMappingSo return translated; } + if (declaringType == typeof(DateOnly) && TranslateDateOnly(instance, member) is { } translated2) + { + return translated2; + } + if (member.Name == nameof(DateTime.Date)) { // Note that DateTime.Date returns a DateTime, not a DateOnly (introduced later); so we convert using date_trunc (which returns @@ -190,13 +195,7 @@ SqlExpression LocalNow() return _sqlExpressionFactory.Convert(result, typeof(int)); } - /// - /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to - /// the same compatibility standards as public APIs. It may be changed or removed without notice in - /// any release. You should only use it directly in your code with extreme caution and knowing that - /// doing so can result in application failures when updating to a new Entity Framework Core release. - /// - public virtual SqlExpression? TranslateDateTimeOffset(SqlExpression instance, MemberInfo member) + private SqlExpression? TranslateDateTimeOffset(SqlExpression instance, MemberInfo member) => member.Name switch { // We only support UTC DateTimeOffset, so DateTimeOffset.DateTime is just a matter of converting to timestamp without time zone @@ -226,6 +225,17 @@ SqlExpression LocalNow() _ => null }; + private SqlExpression? TranslateDateOnly(SqlExpression? instance, MemberInfo member) + => member.Name switch + { + // We use fragment rather than a DateOnly constant, since 0001-01-01 gets rendered as -infinity by default. + // TODO: Set the right type/type mapping after https://github.com/dotnet/efcore/pull/34995 is merged + nameof(DateOnly.DayNumber) when instance is not null + => _sqlExpressionFactory.Subtract(instance, _sqlExpressionFactory.Fragment("DATE '0001-01-01'")), + + _ => null + }; + // Various conversion functions translated here (date_part, ::time) exist only for timestamp without time zone, so if we pass in a // timestamptz it gets implicitly converted to a local timestamp based on TimeZone; that's the wrong behavior (these conversions are not // supposed to be sensitive to TimeZone). diff --git a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMethodTranslator.cs b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMethodTranslator.cs index 101ae9b19..2cb2a5d22 100644 --- a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMethodTranslator.cs +++ b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMethodTranslator.cs @@ -67,6 +67,10 @@ private static readonly MethodInfo DateOnly_AddMonths private static readonly MethodInfo DateOnly_AddYears = typeof(DateOnly).GetRuntimeMethod(nameof(DateOnly.AddYears), [typeof(int)])!; + private static readonly MethodInfo DateOnly_FromDayNumber + = typeof(DateOnly).GetRuntimeMethod( + nameof(DateOnly.FromDayNumber), [typeof(int)])!; + private static readonly MethodInfo TimeOnly_FromDateTime = typeof(TimeOnly).GetRuntimeMethod(nameof(TimeOnly.FromDateTime), [typeof(DateTime)])!; @@ -226,6 +230,18 @@ public NpgsqlDateTimeMethodTranslator( { return _sqlExpressionFactory.MakePostgresBinary(PgExpressionType.Distance, arguments[1], arguments[2]); } + + if (method == DateOnly_FromDayNumber) + { + // We use fragment rather than a DateOnly constant, since 0001-01-01 gets rendered as -infinity by default. + // TODO: Set the right type/type mapping after https://github.com/dotnet/efcore/pull/34995 is merged + return new SqlBinaryExpression( + ExpressionType.Add, + _sqlExpressionFactory.Fragment("DATE '0001-01-01'"), + arguments[0], + typeof(DateOnly), + _typeMappingSource.FindMapping(typeof(DateOnly))); + } } else { diff --git a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs index 39c670f2b..7e2fd3d47 100644 --- a/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.PG/Query/Internal/NpgsqlSqlTranslatingExpressionVisitor.cs @@ -218,21 +218,39 @@ when binaryExpression.Left.Type.UnwrapNullableType().FullName == "NodaTime.Local var translation = base.VisitBinary(binaryExpression); - // A somewhat hacky workaround for #2942. - // When an optional owned JSON entity is compared to null, we get WHERE (x -> y) IS NULL. - // The -> operator (returning jsonb) is used rather than ->> (returning text), since an entity type is being extracted, and further - // JSON operations may need to be composed. However, when the value extracted is a JSON null, a non-NULL jsonb value is returned, - // and comparing that to relational NULL returns false. - // Pattern-match this and force the use of ->> by changing the mapping to be a scalar rather than an entity type. - if (translation is SqlUnaryExpression + switch (translation) + { + // Optimize (x - c) - (y - c) to x - y. + // This is particularly useful for DateOnly.DayNumber - DateOnly.DayNumber, which is the way to express DateOnly subtraction + // (the subtraction operator isn't defined over DateOnly in .NET). The translation of x.DayNumber is x - DATE '0001-01-01', + // so the below is a useful simplification. + // TODO: As this is a generic mathematical simplification, we should move it to a generic optimization phase in EF Core. + case SqlBinaryExpression + { + OperatorType: ExpressionType.Subtract, + Left: SqlBinaryExpression { OperatorType: ExpressionType.Subtract, Left: var left1, Right: var right1 }, + Right: SqlBinaryExpression { OperatorType: ExpressionType.Subtract, Left: var left2, Right: var right2 } + } originalBinary when right1.Equals(right2): + { + return new SqlBinaryExpression(ExpressionType.Subtract, left1, left2, originalBinary.Type, originalBinary.TypeMapping); + } + + // A somewhat hacky workaround for #2942. + // When an optional owned JSON entity is compared to null, we get WHERE (x -> y) IS NULL. + // The -> operator (returning jsonb) is used rather than ->> (returning text), since an entity type is being extracted, and + // further JSON operations may need to be composed. However, when the value extracted is a JSON null, a non-NULL jsonb value is + // returned, and comparing that to relational NULL returns false. + // Pattern-match this and force the use of ->> by changing the mapping to be a scalar rather than an entity type. + case SqlUnaryExpression { OperatorType: ExpressionType.Equal or ExpressionType.NotEqual, Operand: JsonScalarExpression { TypeMapping: NpgsqlOwnedJsonTypeMapping } operand - } unary) - { - return unary.Update( - new JsonScalarExpression( - operand.Json, operand.Path, operand.Type, _typeMappingSource.FindMapping("text"), operand.IsNullable)); + } unary: + { + return unary.Update( + new JsonScalarExpression( + operand.Json, operand.Path, operand.Type, _typeMappingSource.FindMapping("text"), operand.IsNullable)); + } } return translation; diff --git a/src/EFCore.PG/Query/NpgsqlSqlExpressionFactory.cs b/src/EFCore.PG/Query/NpgsqlSqlExpressionFactory.cs index 84fac6794..73c64c92c 100644 --- a/src/EFCore.PG/Query/NpgsqlSqlExpressionFactory.cs +++ b/src/EFCore.PG/Query/NpgsqlSqlExpressionFactory.cs @@ -476,6 +476,18 @@ private SqlBinaryExpression ApplyTypeMappingOnSqlBinary(SqlBinaryExpression bina binary.Type, typeMapping ?? _typeMappingSource.FindMapping(binary.Type, "interval")); } + + // TODO: This is a hack until https://github.com/dotnet/efcore/pull/34995 is done; the translation of DateOnly.DayNumber + // generates a substraction with a fragment, but for now we can't assign a type/type mapping to a fragment. + case ExpressionType.Subtract when left.Type == typeof(DateOnly) && right is SqlFragmentExpression: + { + return new SqlBinaryExpression( + ExpressionType.Subtract, + ApplyDefaultTypeMapping(left), + right, + typeof(int), + _typeMappingSource.FindMapping(typeof(int))); + } } // If this is a row value comparison (e.g. (a, b) > (5, 6)), doing type mapping inference on each corresponding pair. diff --git a/test/EFCore.PG.FunctionalTests/Query/TimestampQueryTest.cs b/test/EFCore.PG.FunctionalTests/Query/TimestampQueryTest.cs index 429c4cc9c..7b6e17486 100644 --- a/test/EFCore.PG.FunctionalTests/Query/TimestampQueryTest.cs +++ b/test/EFCore.PG.FunctionalTests/Query/TimestampQueryTest.cs @@ -744,6 +744,56 @@ WHERE CAST(e."TimestamptzDateTime" AT TIME ZONE 'UTC' AS date) + TIME '15:26:38' """); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task DateOnly_DayNumber(bool async) + { + await AssertQuery( + async, + ss => ss.Set().Where(e => DateOnly.FromDateTime(e.TimestamptzDateTime).DayNumber == 729490)); + + AssertSql( + """ +SELECT e."Id", e."TimestampDateTime", e."TimestampDateTimeArray", e."TimestampDateTimeOffset", e."TimestampDateTimeOffsetArray", e."TimestampDateTimeRange", e."TimestamptzDateTime", e."TimestamptzDateTimeArray", e."TimestamptzDateTimeRange" +FROM "Entities" AS e +WHERE CAST(e."TimestamptzDateTime" AT TIME ZONE 'UTC' AS date) - DATE '0001-01-01' = 729490 +"""); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task DateOnly_DayNumber_subtraction(bool async) + { + await AssertQuery( + async, + ss => ss.Set().Where( + e => DateOnly.FromDateTime(e.TimestamptzDateTime).DayNumber - + DateOnly.FromDateTime(e.TimestamptzDateTime - TimeSpan.FromDays(3)).DayNumber == 3)); + + AssertSql( + """ +SELECT e."Id", e."TimestampDateTime", e."TimestampDateTimeArray", e."TimestampDateTimeOffset", e."TimestampDateTimeOffsetArray", e."TimestampDateTimeRange", e."TimestamptzDateTime", e."TimestamptzDateTimeArray", e."TimestamptzDateTimeRange" +FROM "Entities" AS e +WHERE CAST(e."TimestamptzDateTime" AT TIME ZONE 'UTC' AS date) - CAST((e."TimestamptzDateTime" - INTERVAL '3 00:00:00') AT TIME ZONE 'UTC' AS date) = 3 +"""); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task DateOnly_FromDayNumber(bool async) + { + await AssertQuery( + async, + ss => ss.Set().Where(e => DateOnly.FromDayNumber(e.Id) == new DateOnly(0001, 01, 03))); + + AssertSql( + """ +SELECT e."Id", e."TimestampDateTime", e."TimestampDateTimeArray", e."TimestampDateTimeOffset", e."TimestampDateTimeOffsetArray", e."TimestampDateTimeRange", e."TimestamptzDateTime", e."TimestamptzDateTimeArray", e."TimestamptzDateTimeRange" +FROM "Entities" AS e +WHERE DATE '0001-01-01' + e."Id" = DATE '0001-01-03' +"""); + } + #endregion DateOnly #region TimeOnly