Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/EFCore.PG/Extensions/NpgsqlRangeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,18 @@ public static class NpgsqlRangeExtensions
/// <exception cref="NotSupportedException">{method} is only intended for use via SQL translation as part of an EF Core LINQ query.</exception>
public static NpgsqlRange<T> Except<T>(this NpgsqlRange<T> a, NpgsqlRange<T> b) => throw ClientEvaluationNotSupportedException();

/// <summary>
/// Returns the smallest range which includes both of the given ranges.
/// </summary>
/// <param name="a">The first range.</param>
/// <param name="b">The second range.</param>
/// <typeparam name="T">The type of the elements of <paramref name="a"/>.</typeparam>
/// <returns>
/// The smallest range which includes both of the given rangesge.
/// </returns>
/// <exception cref="NotSupportedException">{method} is only intended for use via SQL translation as part of an EF Core LINQ query.</exception>
public static NpgsqlRange<T> Merge<T>(this NpgsqlRange<T> a, NpgsqlRange<T> b) => throw ClientEvaluationNotSupportedException();

#region Utilities

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public class NpgsqlCompositeMemberTranslator : RelationalCompositeMemberTranslat
[NotNull] [ItemNotNull] static readonly IMemberTranslator[] MemberTranslators =
{
new NpgsqlStringLengthTranslator(),
new NpgsqlDateTimeMemberTranslator()
new NpgsqlDateTimeMemberTranslator(),
new NpgsqlRangeTranslator()
};

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,11 @@ public NpgsqlCompositeMethodCallTranslator(
[NotNull] INpgsqlOptions npgsqlOptions)
: base(dependencies)
{
var instanceTranslators =
new IMethodCallTranslator[]
{
new NpgsqlDateAddTranslator(npgsqlOptions.PostgresVersion),
new NpgsqlMathTranslator(npgsqlOptions.PostgresVersion)
};
var instanceTranslators = new IMethodCallTranslator[]
{
new NpgsqlDateAddTranslator(npgsqlOptions.PostgresVersion),
new NpgsqlMathTranslator(npgsqlOptions.PostgresVersion)
};

// ReSharper disable once DoNotCallOverridableMethodsInConstructor
AddTranslators(MethodCallTranslators);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@

#endregion

using System;
using System.Linq.Expressions;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Query.Expressions;
using Microsoft.EntityFrameworkCore.Query.ExpressionTranslators;
using Npgsql.EntityFrameworkCore.PostgreSQL.Query.Expressions.Internal;
using NpgsqlTypes;

namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query.ExpressionTranslators.Internal
{
Expand All @@ -37,49 +40,100 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query.ExpressionTranslators.Inte
/// <remarks>
/// See: https://www.postgresql.org/docs/current/static/functions-range.html
/// </remarks>
public class NpgsqlRangeTranslator : IMethodCallTranslator
public class NpgsqlRangeTranslator : IMethodCallTranslator, IMemberTranslator
{
/// <inheritdoc />
[CanBeNull]
public Expression Translate(MethodCallExpression expression)
public Expression Translate(MethodCallExpression e)
{
if (expression.Method.DeclaringType != typeof(NpgsqlRangeExtensions))
if (e.Method.DeclaringType != typeof(NpgsqlRangeExtensions))
return null;

switch (expression.Method.Name)
switch (e.Method.Name)
{
case nameof(NpgsqlRangeExtensions.Contains):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "@>", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "@>", typeof(bool));

case nameof(NpgsqlRangeExtensions.ContainedBy):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "<@", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "<@", typeof(bool));

case nameof(NpgsqlRangeExtensions.Overlaps):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&&", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "&&", typeof(bool));

case nameof(NpgsqlRangeExtensions.IsStrictlyLeftOf):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "<<", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "<<", typeof(bool));

case nameof(NpgsqlRangeExtensions.IsStrictlyRightOf):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], ">>", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], ">>", typeof(bool));

case nameof(NpgsqlRangeExtensions.DoesNotExtendRightOf):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&<", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "&<", typeof(bool));

case nameof(NpgsqlRangeExtensions.DoesNotExtendLeftOf):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&>", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "&>", typeof(bool));

case nameof(NpgsqlRangeExtensions.IsAdjacentTo):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "-|-", typeof(bool));
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "-|-", typeof(bool));

case nameof(NpgsqlRangeExtensions.Union):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "+", expression.Arguments[0].Type);
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "+", e.Arguments[0].Type);

case nameof(NpgsqlRangeExtensions.Intersect):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "*", expression.Arguments[0].Type);
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "*", e.Arguments[0].Type);

case nameof(NpgsqlRangeExtensions.Except):
return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "-", expression.Arguments[0].Type);
return new CustomBinaryExpression(e.Arguments[0], e.Arguments[1], "-", e.Arguments[0].Type);

case nameof(NpgsqlRangeExtensions.Merge):
return new SqlFunctionExpression("range_merge", e.Type, new[] { e.Arguments[0], e.Arguments[1] });

default:
return null;
}
}

/// <inheritdoc />
[CanBeNull]
public Expression Translate(MemberExpression e)
{
var type = e.Member.DeclaringType;
if (type == null || !type.IsGenericType || type.GetGenericTypeDefinition() != typeof(NpgsqlRange<>))
return null;

switch (e.Member.Name)
{
case nameof(NpgsqlRange<int>.LowerBound):
{
var lower = new SqlFunctionExpression("lower", e.Type, new[] { e.Expression });

return e.Type.IsNullableType()
? lower
: new SqlFunctionExpression("COALESCE", e.Type, new Expression[] { lower, Expression.Default(e.Type) });
}

case nameof(NpgsqlRange<int>.UpperBound):
{
var upper = new SqlFunctionExpression("upper", e.Type, new[] { e.Expression });

return e.Type.IsNullableType()
? upper
: new SqlFunctionExpression("COALESCE", e.Type, new Expression[] { upper, Expression.Default(e.Type) });
}

case nameof(NpgsqlRange<int>.IsEmpty):
return new SqlFunctionExpression("isempty", e.Type, new[] { e.Expression });

case nameof(NpgsqlRange<int>.LowerBoundIsInclusive):
return new SqlFunctionExpression("lower_inc", e.Type, new[] { e.Expression });

case nameof(NpgsqlRange<int>.UpperBoundIsInclusive):
return new SqlFunctionExpression("upper_inc", e.Type, new[] { e.Expression });

case nameof(NpgsqlRange<int>.LowerBoundInfinite):
return new SqlFunctionExpression("lower_inf", e.Type, new[] { e.Expression });

case nameof(NpgsqlRange<int>.UpperBoundInfinite):
return new SqlFunctionExpression("upper_inf", e.Type, new[] { e.Expression });

default:
return null;
Expand Down
18 changes: 18 additions & 0 deletions src/EFCore.PG/Query/Sql/Internal/NpgsqlQuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public class NpgsqlQuerySqlGenerator : DefaultQuerySqlGenerator
/// </summary>
readonly bool _reverseNullOrderingEnabled;

/// <summary>
/// The type mapping source.
/// </summary>
IRelationalTypeMappingSource TypeMappingSource => Dependencies.TypeMappingSource;

/// <inheritdoc />
protected override string TypedTrueLiteral { get; } = "TRUE::bool";

Expand Down Expand Up @@ -198,6 +203,19 @@ protected override Expression VisitBinary(BinaryExpression expression)
}
}

/// <inheritdoc />
protected override Expression VisitDefault(DefaultExpression e)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this required for exactly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing back a DefaultExpression throws if we don't override. The expression itself is introduced because lower and upper return null on empty or infinite bounds. But since our range types may have non-nullable bounds (e.g. NpgsqlRange<int>.LowerBound is int not int?) we need to coalesce the database null to what the CLR bound ought to be (currently, the default value).

Needing to do this might be another symptom of NpgsqlRange<T> needing some more work... but changes there would be breaking, so my thought was that mitigating the incompatibility here would be reasonable for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this PR is totally fine, this is the only part of this PR that I want to understand a bit better (and have no time today), will look again tomorrow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji Did you still want to take a look at this?

{
// LOWER(range) and UPPER(range) return null on empty or infinite bounds.
// When this happens, we need to ensure the database null is coalesced
// back to the CLR default bound value (e.g. NpgsqlRange<int>.LowerBound is `int` not `int?`).
var instance = e.Type.IsNullableType() ? null : Activator.CreateInstance(e.Type);

Sql.Append(TypeMappingSource.FindMapping(e.Type).GenerateSqlLiteral(instance));

return e;
}

/// <inheritdoc />
protected override Expression VisitUnary(UnaryExpression expression)
{
Expand Down
138 changes: 121 additions & 17 deletions test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,10 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query
/// </summary>
public class RangeQueryNpgsqlTest : IClassFixture<RangeQueryNpgsqlTest.RangeQueryNpgsqlFixture>
{
/// <summary>
/// Provides resources for unit tests.
/// </summary>
RangeQueryNpgsqlFixture Fixture { get; }

/// <summary>
/// Initializes resources for unit tests.
/// </summary>
/// <param name="fixture">The fixture of resources for testing.</param>
public RangeQueryNpgsqlTest(RangeQueryNpgsqlFixture fixture)
{
Fixture = fixture;
Fixture.TestSqlLoggerFactory.Clear();
}

#region Tests

#region Operators

/// <summary>
/// Tests translation for <see cref="NpgsqlRangeExtensions.Contains{T}(NpgsqlRange{T},NpgsqlRange{T})"/>.
/// </summary>
Expand Down Expand Up @@ -490,6 +477,10 @@ public void RangeExceptRange(NpgsqlRange<int> range)
}
}

#endregion

#region User-defined ranges

[Fact]
public void UserDefined()
{
Expand All @@ -501,7 +492,120 @@ public void UserDefined()
}
}

#endregion Tests
#endregion

#region Functions

[Fact]
public void RangeLowerBound()
{
using (RangeContext context = Fixture.CreateContext())
{
try
{
var _ = context.RangeTestEntities.Select(x => x.Range.LowerBound).ToArray();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, does such variables make sense? As I know, there is no reason to make them since the compiler elides them.

Copy link
Copy Markdown
Contributor Author

@austindrenski austindrenski Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps Rider/ReSharper from complaining about pure functions going unused, and IIRC _ is viewable when stepping through the debugger.

So yes, functionally they make no difference, but I would prefer to keep them for testing/debugging purposes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's leave it for Rider.

Copy link
Copy Markdown
Member

@roji roji Aug 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austindrenski all of the Rider/Resharper complaints can be silenced via pragmas/comments, which seems like a better way to handle this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually scratch that, your way is better :)

}
catch
{
AssertContainsSql("SELECT COALESCE(lower(x.\"Range\"), 0)");
}

AssertContainsSql("SELECT COALESCE(lower(x.\"Range\"), 0)");
}
}

[Fact]
public void RangeUpperBound()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.UpperBound).ToArray();
AssertContainsSql("SELECT COALESCE(upper(x.\"Range\"), 0)");
}
}

[Fact]
public void RangeIsEmpty()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.IsEmpty).ToArray();
AssertContainsSql("SELECT isempty(x.\"Range\")");
}
}

[Fact]
public void RangeLowerBoundIsInclusive()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.LowerBoundIsInclusive).ToArray();
AssertContainsSql("SELECT lower_inc(x.\"Range\")");
}
}

[Fact]
public void RangeUpperBoundIsInclusive()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.UpperBoundIsInclusive).ToArray();
AssertContainsSql("SELECT upper_inc(x.\"Range\")");
}
}

[Fact]
public void RangeLowerBoundInfinite()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.LowerBoundInfinite).ToArray();
AssertContainsSql("SELECT lower_inf(x.\"Range\")");
}
}

[Fact]
public void RangeUpperBoundInfinite()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.UpperBoundInfinite).ToArray();
AssertContainsSql("SELECT upper_inf(x.\"Range\")");
}
}

[Fact]
public void RangeMergeRange()
{
using (RangeContext context = Fixture.CreateContext())
{
var _ = context.RangeTestEntities.Select(x => x.Range.Merge(x.Range)).ToArray();
AssertContainsSql("SELECT range_merge(x.\"Range\", x.\"Range\")");
}
}

#endregion

#endregion

#region Setup

/// <summary>
/// Provides resources for unit tests.
/// </summary>
RangeQueryNpgsqlFixture Fixture { get; }

/// <summary>
/// Initializes resources for unit tests.
/// </summary>
/// <param name="fixture">The fixture of resources for testing.</param>
public RangeQueryNpgsqlTest(RangeQueryNpgsqlFixture fixture)
{
Fixture = fixture;
Fixture.TestSqlLoggerFactory.Clear();
}

#endregion

#region TheoryData

Expand Down Expand Up @@ -674,7 +778,7 @@ public class RangeContext : DbContext
/// <param name="options">
/// The options to be used for configuration.
/// </param>
public RangeContext(DbContextOptions options) : base(options) { }
public RangeContext(DbContextOptions options) : base(options) {}

/// <inheritdoc />
protected override void OnModelCreating(ModelBuilder builder)
Expand Down