Map range operations for NpgsqlRange<T> #323

austindrenski · 2018-03-04T22:48:36Z

Map range operations for `NpgsqlRange<T>`

Work in progress. See #311 for more information.

Implemented:

Adds extension methods with PostgreSQL translation:

bool Contains<T>(NpgsqlRange<T> range, T value) where T : IComparable<T> {...}
// WHERE range @> value

bool Contains<T>(NpgsqlRange<T> range, NpgsqlRange<T> value) where T : IComparable<T> {...}
// WHERE range @> value

bool ContainedBy<T>(NpgsqlRange<T> value, NpgsqlRange<T> range) where T : IComparable<T> {...}
// WHERE value <@ range

bool Overlaps<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a && b

bool IsStrictlyLeftOf<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a << b

bool IsStrictlyRightOf<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a >> b

bool DoesNotExtendRightOf<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a &< b

bool DoesNotExtendLeftOf<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a &> b

bool IsAdjacentTo<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a -|- b

NpgsqlRange<T> Union<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a + b

NpgsqlRange<T> Intersect<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a * b

NpgsqlRange<T> Except<T>(NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T> {...}
// WHERE a - b

Planned:

Implement in-memory functionality for range operators:
- https://www.postgresql.org/docs/current/static/functions-range.html
Add unit test coverage
Look at whether properties on NpgsqlRange<T> translate into their Postgres functions.
- e.g. range.LowerBound -> lower(range)

Issues:

~~range @> range generates SQL like range @> range = TRUE~~
- Known: DefaultQuerySqlGenerator not flexible enough for provider override dotnet/efcore#9143
NpgsqlRange<T> should implement IComparable<T>
- This is opt-in on the part of Npgsql. If/when implemented, the translation support exists.
The equality methods on NpgsqlRange<T> are returning unexpected results.

roji

Thanks for this fast and high-quality work, it looks great!

I'm not 100% sure of the value of providing in-memory implementations of the range operations. For general, non-PostgreSQL-specific types/operations (like string regex), this makes a lot of sense; it allows the same code to client-evaluate on one database and server-evaluate on another. However, NpgsqlRange is by its nature limited to PostgreSQL only, and I'm not sure where and why someone would want to do these operations in-memory... What do you think?
We can decide to make IComparable<T> IComparable<T>, but I think we should do it only if we end up providing in-memory implementations of all operations, which I'm not sure about (see above). In any case, the value of this would be somewhat limited, considering we can express comparison with the other operations (greater than, lesser than).
Regarding range @> range = TRUE, see DefaultQuerySqlGenerator not flexible enough for provider override dotnet/efcore#9143. There's nothing to be done about it at the moment, but there's also no real issue with the = TRUE either.

roji · 2018-03-05T05:10:10Z

src/EFCore.PG/EFCore.PG.csproj

@@ -1,5 +1,4 @@
 <Project Sdk="Microsoft.NET.Sdk">
-
  <PropertyGroup>
    <VersionPrefix>2.1.0-preview2</VersionPrefix>


Can you please undo the whitespace changes to the csproj?

IDE settings gone awry...fixed

roji · 2018-03-05T05:13:31Z

src/EFCore.PG/NpgsqlDbFunctionsExtensions.cs

@@ -27,6 +27,7 @@
 using System.Text;
 using System.Text.RegularExpressions;
 using JetBrains.Annotations;
+using NpgsqlTypes;


Please remove

roji · 2018-03-05T05:14:40Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeOperatorTranslator.cs

+        /// <summary>
+        /// Caches runtime method information for <see cref="NpgsqlRangeExtensions.Equal{T}(NpgsqlRange{T}, NpgsqlRange{T})"/>.
+        /// </summary>
+        private static readonly MethodInfo Equal;


Nit: I don't put private as it's the default, the code is terser that way.

Removed default modifiers

roji · 2018-03-05T05:15:28Z

test/EFCore.PG.FunctionalTests/EFCore.PG.FunctionalTests.csproj

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="2.1.0-preview2-30183" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="2.1.0-preview2-30183" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Relational.Specification.Tests" Version="2.1.0-preview2-30183" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="2.1.0-preview2-30183" />
    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="2.1.0-preview2-30183" />
  </ItemGroup>
-
-</Project>
+</Project>


Please undo

IDE settings gone awry...fixed

roji · 2018-03-05T05:16:46Z

src/EFCore.PG/NpgsqlRangeExtensions.cs

+        [Pure]
+        public static bool Equal<T>(this NpgsqlRange<T> a, NpgsqlRange<T> b) where T : IComparable<T>
+        {
+            throw new NotImplementedException();


Please convert all methods in this file to be expression-bodied

Single-line methods converted

roji · 2018-03-05T05:46:46Z

src/EFCore.PG/Query/Sql/Internal/NpgsqlQuerySqlGenerator.cs

@@ -152,7 +159,7 @@ void GenerateArrayIndex([NotNull] BinaryExpression expression)
            {
                // bytea cannot be subscripted, but there's get_byte
                VisitSqlFunction(new SqlFunctionExpression("get_byte", typeof(byte),
-                    new[] { expression.Left, expression.Right }));
+                                                           new[] { expression.Left, expression.Right }));


Please undo

IDE strikes again -- reverted

roji · 2018-03-05T05:46:51Z

src/EFCore.PG/Query/Sql/Internal/NpgsqlQuerySqlGenerator.cs

@@ -162,7 +169,7 @@ void GenerateArrayIndex([NotNull] BinaryExpression expression)
                // PostgreSQL substr() is 1-based.

                VisitSqlFunction(new SqlFunctionExpression("substr", typeof(char),
-                    new[] { expression.Left, expression.Right, Expression.Constant(1) }));
+                                                           new[] { expression.Left, expression.Right, Expression.Constant(1) }));


Please undo

IDE strikes again -- reverted

roji · 2018-03-05T05:47:13Z

src/EFCore.PG/Query/Sql/Internal/NpgsqlQuerySqlGenerator.cs

@@ -206,14 +230,17 @@ public Expression VisitRegexMatch([NotNull] RegexMatchExpression regexMatchExpre
            }

            Sql.Append("('(?");
-            if (options.HasFlag(RegexOptions.IgnoreCase)) {
+            if (options.HasFlag(RegexOptions.IgnoreCase))
+            {


Please undo

IDE strikes again -- reverted

roji · 2018-03-05T05:50:04Z

test/EFCore.PG.FunctionalTests/Query/RangeContainedByNpgsqlQueryTest.cs

@@ -0,0 +1,173 @@
+using System.Linq;


Let's unify all the range tests into one RangeQueryNpgsqlTest, there's simply not enough of them to clutter the Query folder. You can still use regions inside to distinguish between the different operations etc.

Moved tests into RangeQueryNpgsqlTest

roji · 2018-03-05T05:51:11Z

test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs

@@ -44,8 +43,7 @@ public void GenerateSqlLiteral_returns_timestamptz_datetime_literal()
                mapping.GenerateSqlLiteral(new DateTime(1997, 12, 17, 7, 37, 16, DateTimeKind.Unspecified)));

            var offset = TimeZoneInfo.Local.BaseUtcOffset.Hours;
-            var offsetStr = offset < 10 ? $"0{offset}" : offset.ToString();
-            Assert.StartsWith($"TIMESTAMPTZ '1997-12-17 07:37:16+{offsetStr}",
+            Assert.StartsWith($"TIMESTAMPTZ '1997-12-17 07:37:16{offset:+00;-00}",


Can you please take this commit out of this PR? I'll review #322 separately...

Sure -- reverted

roji · 2018-03-05T14:56:36Z

OK great, let me know when you want me to take another look.

austindrenski · 2018-03-05T16:00:19Z

Updates

I've pushed some changes to address the white-space/brace/modifiers/unit test changes.
- My IDE settings were a little more aggressive than I realized
- I'll try to keep an eye on the white-space and blank line diffs going forward

In-memory implementations:

I'm not 100% sure of the value of providing in-memory implementations of the range operations. For general, non-PostgreSQL-specific types/operations (like string regex), this makes a lot of sense; it allows the same code to client-evaluate on one database and server-evaluate on another. However, NpgsqlRange is by its nature limited to PostgreSQL only, and I'm not sure where and why someone would want to do these operations in-memory... What do you think?

I'm in favor of providing in-memory implementations for a handful of reasons:
1. I have a handful of (personal) use cases where in-memory evaluation would be helpful:
  - e.g. validating user input on the server (web API) before saving back to Postgres
2. Simplification:
  - No need to document "this works here, but only if here is evaluated in Postgres"
3. Makes REPL workflows possible with the Npgsql types:
  - Testing with C# interactive
  - Might matter more for F# workflows
    - Is there a significant F# community using Npgsql?
Implicit in the above is that I will rework the in-memory implementations so that they:
1. Exist
2. Work correctly
3. Follow Postgres semantics
  - Review whether any of the semantics change across the Npgsql-supported versions of Postgres

`IComparable<T>`:

We can decide to make IComparable IComparable, but I think we should do it only if we end up providing in-memory implementations of all operations, which I'm not sure about (see above). In any case, the value of this would be somewhat limited, considering we can express comparison with the other operations (greater than, lesser than).

I've reworked the ==/Equals(...) and related comparison symbols to use the built-ins rather than my extension methods. If it works as intended, this should make the comparison operators opt-in on the part of Npgsql.

Currently, the compiler complains if < is used on NpgsqlRange<T>. If we add IComparable<T> to NpgsqlRange<T>, then < will be intercepted and rewritten as a RangeOperatorExpression.

Provided my "opt-in" code is working right, this punts the question to the Npgsql project.

If you think this is worth supporting, I could take a first pass at adding IComparable<T> support on the Npgsql side after everything is wrapped up here.

`range @> range = TRUE`:

Regarding range @> range = TRUE, see dotnet/efcore#9143. There's nothing to be done about it at the moment, but there's also no real issue with the = TRUE either.

Great! I wanted to make sure I hadn't missed some code telling EF Core "this operator returns boolean".

Aesthetically, it would be nice to remove, but looks like the ball is out of our court.

My next steps

Review the method caching and equality in NpgsqlRangeOperatorTranslator
Review the existing in-memory implementations in NpgsqlRangeExtensions
Expand unit test coverage

roji · 2018-03-05T16:42:51Z

Thanks for all the changes and the reaction speed.

I'm in favor of providing in-memory implementations for a handful of reasons:

I have a handful of (personal) use cases where in-memory evaluation would be helpful:

e.g. validating user input on the server (web API) before saving back to Postgres

This kind of thing is frequently done by settings constraints on the PostgreSQL side (i.e. that a given value is within the range), but I imagine in some scenarios some in-memory operations could be useful.

Simplification:

No need to document "this works here, but only if here is evaluated in Postgres"

I don't think that's quite necessary - we're discussing extension methods that come from the Npgsql EF Core provider, it's quite clear that their first purpose is mapping to SQL operations. There are also several other cases where similar operations throw.

In parallel to this there's an implementation of PostgreSQL text search (#315). Short of an in-memory reimplemention of the entire whole text search engine, including linguistic capabilities, methods there will have to throw when executed in memory... Of course the complexity of ranges is nowhere near that of full text search, but this is a good example of where an in-memory implementation simply isn't relevant.

Makes REPL workflows possible with the Npgsql types:

Testing with C# interactive

Might matter more for F# workflows

Is there a significant F# community using Npgsql?

OK. Don't forget that you can still do EF Core in REPL, but sure, if you're just playing around with the types and their implementation then it's useful to have in-memory.

Implicit in the above is that I will rework the in-memory implementations so that they:

Exist

Work correctly

Follow Postgres semantics

Review whether any of the semantics change across the Npgsql-supported versions of Postgres

That's an important point - if the in-memory implementation is going to differ in any way from the PostgreSQL implementation, then it's better not to have it at all (better to have a clear exception than a subtle behavior change).

One more objection I haven't made before... Keep in mind that in theory PostgreSQL allows user-defined ranges over any type - including types which on the CLR side may be mapped to non-comparable types. It's true that at this point only the built-in range types are supported (int4range, int8range, numrange, tsrange, tstzrange, daterange), but that may change in the future.

Another even worst possibility is for the ordering of element types to be somehow subtly different between CLR and PostgreSQL, so that the in-memory implementation would return different results than the PostgreSQL one - this would be totally beyond your control.

So there are risks to having an in-memory implementation. That being said, for ranges specifically I think the complexity and risks are lower than other cases, and if you want to fully implement this I'm OK with that.

I've reworked the ==/Equals(...) and related comparison symbols to use the built-ins rather than my extension methods. If it works as intended, this should make the comparison operators opt-in on the part of Npgsql.

OK, but let's distinguish between equality (==/Equals) and comparison (IComparable<T>). For equality, it's important to translate ==, Equals, and also IEquatable<T>, which is generic and allows comparing two ranges without boxing (NpgsqlRange is a struct).

Currently, the compiler complains if < is used on NpgsqlRange. If we add IComparable to NpgsqlRange, then < will be intercepted and rewritten as a RangeOperatorExpression.

There's something I may be missing... If you add IComparable<T> in Npgsql, then you have to also provide the in-memory implementation to those methods in Npgsql, don't you?

This can lead to a different way of looking at this... If you really intend to provide all the in-memory implementations as said above, that can be done on the NpgsqlRange<T> type itself, rather than via extension methods in the EF Core provider. The EF Core provider would still identify these and translate them to their SQL counterparts, of course. This would provide the benefits of in-memory evaluation even when not using EF Core.

What do you think? If you want to go down this route I think I'd be fine with that. We would probably release this in a patch version of 3.2, to allow EF Core 2.1 to still use Npgsql 3.2 and not force a dependency on 3.3.

Great progress so far!

roji · 2018-03-06T07:01:09Z

src/EFCore.PG/NpgsqlRangeExtensions.cs


-            bool testLower = compareLower > 0 || compareLower == 0 && range.LowerBoundIsInclusive;
-            bool testUpper = compareUpper > 0 || compareUpper == 0 && range.UpperBoundIsInclusive;
+            bool lower = a.LowerBoundInfinite || !b.LowerBoundInfinite && compareLower > 0 || compareLower == 0 && a.LowerBoundIsInclusive || !b.LowerBoundIsInclusive;


I personally add parentheses in cases like this because I'm incapable of remembering precedence and it's better to be 100% unambiguous...

austindrenski · 2018-03-07T06:49:23Z

This most recent commit reworks the unit tests to run through a wider range of cases.

In doing so, I found that one of the containment cases fails when the containing range (e.g. [1,10)) has an inclusive lower bound that is the first incremented value after the non-inclusive lower bound of the contained range (e.g. (0,5)).

This query returns true (version 9.6):

SELECT '[1,10)'::int4range @> '(0,5)'::int4range;

This seems like it will be difficult to match with purely generic functions.

Some thoughts:

Specific types (e.g. int, datetime) could use non-generic overloads to allow for the increment.
Perhaps there's some handling that could be done within NpgsqlRange<T> to preempt this situation?

Hoping that I've missed the obvious solution... Any ideas?

roji · 2018-03-07T07:07:46Z

I understand. The situation seems even more complex, since whether this containment is true depends on whether the underlying type is discrete or not... In other words, for a range of float/double the above would be false, since '(0,5)' includes 0.5, which isn't included in [1,5)...

One way around this without specific checks inside those methods (i.e. if (typeof(T) == typeof(int)), cast T to the appropriate value and then increment, since .NET simply doesn't provide a concept of increment generically. That solution doesn't seem too bad, even if it won't take care of the totally of possible user-defined ranges (but that's largely theoretical).

But this seems like a lot of trouble for an in-memory implementation, and there's also a good risk of some edge case discrepancies... I'm even less sure now that it's a good idea to do this.

austindrenski · 2018-03-07T15:27:31Z

I agree that the edge case risk is significant...

While I still want to see in-memory support, the known edge cases will take time to get right (and I'm sure that others will be found in the process).

To keep things moving for EF Core 2.1, I'll split off the in-memory implementations from the translations, and move the current in-memory work into a new branch, range-operators-in-memory. The current implementations will be rewritten to throw on NotSupportedExceptions.

I'll submit separate PRs as each in-memory operator achieves parity with the Postgres version. I think this will make it easier to spot and contain edge cases during review.

Next steps

I'll push a commit in the next few minutes to make the changes described above.
Then I'll begin adding unit tests to complete coverage of the operator extensions.

roji · 2018-03-07T16:31:05Z

While I still want to see in-memory support, the known edge cases will take time to get right (and I'm sure that others will be found in the process).

To keep things moving for EF Core 2.1, I'll split off the in-memory implementations from the translations, and move the current in-memory work into a new branch, range-operators-in-memory. The current implementations will be rewritten to throw on NotSupportedExceptions.

I think that's a good idea. The one problem is that if you are going to do in-memory, there's the option of putting the methods on the NpgsqlRange type itself - inside Npgsql - rather than as extensions (I mentioned this possibility above). Of course this makes no sense if we end up dropping in-memory.

But in any case, we can start like this and see where it leads us.

roji · 2018-03-10T05:54:58Z

@austindrenski don't forget to ping me when you want me to review again.

roji · 2018-03-27T14:33:45Z

@austindrenski EF Core 2.1 is around the corner, and I'd like to close whatever outstanding things are left. Any chance we can quickly work on merging this? If you have no time to work on it, let me know and I'll continue the work myself.

roji · 2018-05-14T04:32:16Z

@austindrenski, I can see some activity on this PR, but it currently lists 104 commits... This is probably a result of a problematic merge.

The best thing to do would be to take the current Npgsql dev branch as the new base for your PR branch, and cherry-pick your changes on top of it. The PR should show only the commits you've added to implement this functionality. Post back if this is confusing and you need git guidance.

austindrenski · 2018-05-14T04:52:44Z

@roji, It looks like I missed your ping from back in March, apologies for that.

As for all the commits... some git guidance on cleaning up the PR would be much appreciated. I seem to have gone a little awry while bringing my branch up to date...

roji · 2018-05-14T05:20:42Z

So the goal is to update your PR branch, range-operators, to be based on top of the latest EFCore.PG dev branch and add your commit(s).

First, I'd "back up" your current range-operators to make sure you don't lose anything: git tag range-operators-backup. You can delete that later with git tag -d range-operators-backup.

Then, I'd checkout the dev branch, make sure it's up to date (git pull origin dev), and overwrite your range-operators branch by doing git checkout -B range-operators. This clobbers your branch and your work would be "lost" (that's why we created the tag earlier).

At this you can cherry-pick your commit where you did actual work by doing git cherry-pick <commit SHA1>. You can use git log range-operators-backup to see the previous list. You will likely have some conflicts - this is normal as EFCore.PG has progressed since you first did your work. You'll have to resolve these, and at the end your range-operators branch will be based off of the latest dev, plus your commit.

At this point you can update the PR on github simply by force-pushing your branch to github: git push -f <your_repo_on_github> range-operators. Github will automatically pick that up and update the PR here.

Once everything is good, you can delete your older work via git tag -d range-operators-backup, but make sure you're not losing anything.

austindrenski · 2018-05-14T07:31:05Z

I appreciate the guidance, but I'm still having trouble resolving the conflicts on my end while going through the cherry-picking process.

Is there any harm from to the PR process in rebasing and...manually recreating the changes prior to the force push?

roji · 2018-05-14T08:06:18Z

No harm in doing anything - whatever it is you end up doing, in the end you should have a clean local range-operators branch which you'll push-force to github in order to update this PR. It doesn't really matter how you do it.

…ange operators with extension methods and provides hooks for consumers to include local evaulation implementations. Adds basic tests to verify SQL generation.

austindrenski · 2018-05-14T14:26:52Z

Alright, I think the rebase and change incorporation went through okay.

Changes since the last round of commits:

I reworked some of the implementation code and defined NpgsqlOperatorAttribute to store translation metadata directly on the NpgsqlRangeOperatorType enum. It reduces the number of places that record translation info and (hopefully) simplify future maintenance.
I also defined NpgsqlRangeOperatorAttribute to simplify the identification and mapping of extension methods to operators. This provides a hook to make other methods eligible for translation as range operators. The motivation here is to keep some flexibility for future additions.
- For example:
  - Migrating extension methods in the EF Core project to instance methods on NpgsqlRange<T>.
  - Consumers providing implementations for local evaluation without modifying the translation code.

roji · 2018-05-14T14:32:32Z

Thanks @austindrenski, I'll try to review this ASAP. In the meantime, I suggest you take a look at CustomBinaryExpression, which is perfect exactly for this kind of case (rather than having to define a new expression type just for range operations). It wasn't there when you originally started working, it would probably cut down on quite a bit of code.

I'll do a proper review soon.

austindrenski · 2018-05-14T16:34:41Z

I took a look at CustomBinaryExpression and it drastically reduces the amount of expression code.

I'm wrapping up a refactor now. If you haven't started yet, I recommend waiting until these commits are pushed.

…ew range operator methods for translation (e.g. client code can define and register a method that supports client evaluation). The goal is to build on CustomBinaryExpression and make it easier to add new operators (e.g. XML operators from issue #7). With the current structure, additional operator classes can be codified in 3 files: Npgsql{type_name}OperatorType, Npgsql{type_name}OperatorAttribute, and Npgsql{type_name}OperatorTranslator.

austindrenski · 2018-05-14T20:50:57Z

I finished the refactoring to use CustomBinaryExpression, and I think it's in good shape for review.

I tried to keep things extensible with NpgsqlRangeOperatorTranslator by providing a hook for other methods to opt into range operator translation. Currently, this works by:

Declaring an instance of NpgsqlRangeOperatorAttribute on the method.
Registering the method with NpgsqlRangeOperatorTranslator.AddSupportedMethod(MethodInfo).

To support these hooks, I moved NpgsqlRangeOperatorType and NpgsqlRangeOperatorAttribute from Npgsql.EntityFrameworkCore.PostgreSQL.Query.ExpressionTranslators.Internal to Npgsql.EntityFrameworkCore.PostgreSQL.Query.ExpressionTranslators.

roji

@austindrenski I reviewed your PR, thanks again for your work. Apart from the specific comments below, here are some more general ones.

I understand what you're trying to do with NpgsqlBinaryOperatorAttribute, NpgsqlRangeOperatorAttribute and the "dynamic" discovery of extension methods, but I honestly think it's a little over-engineered for this specific case:

There's simply a great number of different classes to support this mechanism: a NpgsqlRangeOperator attribute, which accepts an NpgsqlRangeOperatorType enum, which is annotated with an NpgsqlBinaryOperator, with all this needing to be processed by the translator.
This sort of "generic" programming frequently creates issues such as this one, where you end up making assumptions which may not hold.
A typical translator simply knows the methods it translates, and contains all the logic for translating them. This concentrates all the logic in a single, clear place, and allows you to handle different method calls in a special way if needed (and it's frequently needed in EF Core translators). A good example is the full text operation translator, where we have a simple switch that creates the CustomBinaryExpressions. You can also look at the NetTopologySuite translator (still WIP).
Another problem I have with this is that it mixes different concerns together architecturally. The extension methods in NpgsqlRangeExtensions are pure .NET methods, which aren't supposed to be concerned with how they're rendered into SQL (in some cases they even contain a C# implementation). Putting
Finally, the attribute-based mechanism is only relevant for extension methods added by the EF Core provider, but it also has to translate completely external methods - BCL methods, methods from other libraries (e.g. NetTopologySuite), etc. I think it's healthier to have one simple way to translate, rather than have a special attribute-based mechanism for Npgsql-defined extension methods, and another mechanism for the others.

On another note... I'm not sure I see the need for the extensibility features on NpgsqlRangeOperatorTranslator - who exactly is going to be adding and removing methods via this API? If in the future we add any methods in NpgsqlRangeExtensions (or even elsewhere), at the same time we'd commit changes to the translator to recognize these methods. It's also worth mentioning that EF Core already provides a more general extensibility model by using dependency injection: if we want to translate other methods, we can do so by adding another translator to NpgsqlCompositeMethodCallTranslator. In fact, this is exactly what the new plugin model does in 2.1 (not yet merged but visible in https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/tree/plugins).

To summarize, the work is very good - I'm just concerned with the factoring/architecture, which should probably be much simpler. I propose you look at some other translators, and if you still disagree we can definitely discuss.

PS The comprehensive test suite is really good, thanks for that.

roji · 2018-05-15T09:18:58Z

src/EFCore.PG/NpgsqlRangeExtensions.cs

+        /// <summary>
+        /// Determines whether a range contains a specified value.
+        /// </summary>
+        /// <param name="range">


nit: here and below, why not put the <param> and <typeparam> in a single line:

/// <param name="range">The range in which to locate the value</param>

This seems to be standard in the .NET / EF Core code bases and reduces lots of useless lines

roji · 2018-05-15T09:22:32Z

src/EFCore.PG/NpgsqlRangeExtensions.cs

+        /// <value>true</value> if the range contains the specified value; otherwise, <value>false</value>.
+        /// </returns>
+        [NpgsqlRangeOperator(NpgsqlRangeOperatorType.Contains)]
+        public static bool Contains<T>(this NpgsqlRange<T> range, T value) where T : IComparable<T> => throw new NotImplementedException();


I think this should throw NotSupportException, not NotImplementedException.

roji · 2018-05-15T09:26:27Z

test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs

+                           .Where(x => !x.Range.Contains(range))
+                           .ToArray();
+
+                Assert.Contains("WHERE NOT (x.\"Range\" @> @__range_0 = TRUE)", Fixture.TestSqlLoggerFactory.Sql);


Nit: Refactor this out to a single AssertContainsSql() which can be called from all the tests.

roji · 2018-05-15T09:29:58Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeOperatorTranslator.cs

+    /// By default, this class supports translation for methods declared on <see cref="NpgsqlRangeExtensions"/>.
+    /// Additional methods can be supported so long as they declare a <see cref="NpgsqlRangeOperatorAttribute"/>.
+    /// </remarks>
+    public class NpgsqlRangeOperatorTranslator : IMethodCallTranslator


Please rename to NpgsqlRangeTranslator to align with the naming scheme of other translators.

roji · 2018-05-15T09:43:31Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlBinaryOperatorAttribute.cs

+            Check.NotNull(right, nameof(right));
+
+            // TODO: will the left type arguments always be valid?
+            Type type = ReturnType.IsGenericType ? ReturnType.MakeGenericType(left.Type.GetGenericArguments()) : ReturnType;


nit: please use var everywhere

roji · 2018-05-15T09:45:54Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlBinaryOperatorAttribute.cs

+            Check.NotNull(left, nameof(left));
+            Check.NotNull(right, nameof(right));
+
+            // TODO: will the left type arguments always be valid?


See my general comment about the architecture. Simply moving all the logic into the translator makes this go away, as you'd be translating each operation separately and can make this decision on a per-operation basis.

austindrenski · 2018-05-15T13:40:40Z

@roji Thanks for the prompt review. I've pushed a commit addressing your comments, and it's ready for another round of review.

There's simply a great number of different classes to support this mechanism

I think it's healthier to have one simple way to translate, rather than have a special attribute-based mechanism for Npgsql-defined extension methods, and another mechanism for the others.

Those are very fair points. Perhaps too much time with ASP.NET Core has made every problem look well-suited for an attribute-based solution.

Incorporating your comments eliminated a bunch of complexity and removed 3 extra files.

Appreciate all the feedback!

roji

@austindrenski thanks for the changes - this looks much better. This is really close for merging, see my last minor comments for further simplifying the code.

PS And thanks for taking my previous comments in a positive way. I have the same tendency for over-engineering things, I recognized myself there for a moment :)

roji · 2018-05-15T16:22:32Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs

+        /// The expression if successful; otherwise, null.
+        /// </returns>
+        [CanBeNull]
+        static Expression TryTranslateOperator([NotNull] MethodCallExpression expression)


Where not merge TryTranslateOperator with Translate above, to just have the entire switch in Translate()? The separation doesn't seem to add any value. In addition, methods start with Try in C# normally return a bool (success/failure) and have an out var, so this method isn't really following standard naming.

roji · 2018-05-15T16:23:14Z

src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs

+        /// A <see cref="CustomBinaryExpression"/>.
+        /// </returns>
+        [NotNull]
+        static Expression MakeBinaryExpression([NotNull] MethodCallExpression expression, [NotNull] string symbol, [NotNull] Type returnType) =>


Similar to the above, why not just instantiate CustomBinaryExpression inside the switch, rather than calling this method which does it? There's almost zero value added here.

roji · 2018-05-15T16:25:01Z

test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlFixture.cs

+    /// <summary>
+    /// Represents a fixture suitable for testing range operators.
+    /// </summary>
+    public class RangeQueryNpgsqlFixture : IDisposable


In EF Core the standard now seems to be to include the fixture as an inner class of the test suite class. This makes sense since the fixture is only used within its test suite, and this way we have one file per test suite - easier to navigate the ever-growing list of test suites. Let's do this here as well.

austindrenski · 2018-05-15T17:22:28Z

@roji The Translate/TryTranslateOperator was a relic of some earlier iteration for caching methods/attributes at runtime... No real purpose now, so collapsed per your comment.

I also relocated the fixture classes and removed that file. Given the size of these test suites, happy to see EF Core projects move that direction.

roji

Perfect, thanks for your contribution and your patience.

Bug fix for 502ce2a. NpgsqlRangeTranslator uses nameof(...) without checking the declaring type. This returns the unqualified name, such that all methods named Contains that reach this translator are handled. This commit adds a check to screen for the declaring type.

When a custom binary operator uses a standard comparison operator (=, <>, <, >, <=, >=) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: WHERE a < b = TRUE This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. While it seems like this could be avoided by having the custom translator construct a standard Expression.LessThan(Expression,Expression), this causes an error to be thrown...because the binary operator isn't defined. See the related links for more information. Example stack trace: System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) Related: - dotnet/efcore#9143 - npgsql#323 (review)

When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard Expression.LessThan(Expression,Expression), this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - [](dotnet/efcore#9143) - [](npgsql#323 (review))

When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard `Expression.LessThan(Expression,Expression)`, this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - dotnet/efcore#9143 - npgsql#323 (review)

When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard `Expression.LessThan(Expression,Expression)`, this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - dotnet/efcore#9143 - #323 (review)

roji reviewed Mar 5, 2018

View reviewed changes

roji reviewed Mar 6, 2018

View reviewed changes

Rebase and manual touchup. Includes work for translating PostgreSQL r…

4e1820d

…ange operators with extension methods and provides hooks for consumers to include local evaulation implementations. Adds basic tests to verify SQL generation.

austindrenski force-pushed the range-operators branch from 7661b77 to 4e1820d Compare May 14, 2018 14:10

austindrenski added 2 commits May 14, 2018 12:59

Refactoring to use CustomBinaryExpression.

403908a

austindrenski mentioned this pull request May 14, 2018

Support inet/cidr operators #299

Closed

roji requested changes May 15, 2018

View reviewed changes

austindrenski added 2 commits May 15, 2018 09:07

Addressing code review comments (2018-05-15).

7fb701c

Fixing up some annotations for consistency.

ed0b754

roji requested changes May 15, 2018

View reviewed changes

Addressing code review comments #2 (2018-05-15).

fc9ff22

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 15, 2018

Incorporating feedback from range-operators PR (npgsql#323).

600c890

roji approved these changes May 15, 2018

View reviewed changes

roji merged commit 502ce2a into npgsql:dev May 15, 2018

austindrenski deleted the range-operators branch May 15, 2018 17:42

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 15, 2018

Incorporating feedback from range-operators PR (npgsql#323).

8e654d3

austindrenski mentioned this pull request May 15, 2018

Bug fix for #323 #408

Merged

austindrenski mentioned this pull request May 15, 2018

Map network type operations #407

Merged

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 15, 2018

Incorporating feedback from range-operators PR (npgsql#323).

f87d862

austindrenski mentioned this pull request May 16, 2018

Updating docs for range operators #409

Merged

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 17, 2018

Incorporating feedback from range-operators PR (npgsql#323).

9dbc029

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 24, 2018

Incorporating feedback from range-operators PR (npgsql#323).

2573c1f

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 25, 2018

Incorporating feedback from range-operators PR (npgsql#323).

2c9cf8c

austindrenski mentioned this pull request May 25, 2018

Patch for custom binary operators that use standard comparison symbols #426

Merged

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 26, 2018

Incorporating feedback from range-operators PR (npgsql#323).

d1e9043

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 27, 2018

Incorporating feedback from range-operators PR (npgsql#323).

e01d724

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request May 29, 2018

Incorporating feedback from range-operators PR (npgsql#323).

1ee4794

austindrenski mentioned this pull request Jul 22, 2018

Support for statistical aggregate functions #532

Closed

austindrenski self-assigned this Aug 16, 2018

austindrenski added the enhancement New feature or request label Aug 16, 2018

austindrenski mentioned this pull request Aug 23, 2018

Translate simple range operators + compatibility support #616

Merged

Map range operations for NpgsqlRange<T> #323

Map range operations for NpgsqlRange<T> #323

Conversation

austindrenski commented Mar 4, 2018 • edited Loading

Map range operations for NpgsqlRange<T>

Implemented:

Planned:

Issues:

roji left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Mar 5, 2018

austindrenski commented Mar 5, 2018

Updates

In-memory implementations:

IComparable<T>:

range @> range = TRUE:

My next steps

roji commented Mar 5, 2018

Choose a reason for hiding this comment

austindrenski commented Mar 7, 2018

roji commented Mar 7, 2018

austindrenski commented Mar 7, 2018

Next steps

roji commented Mar 7, 2018

roji commented Mar 10, 2018

roji commented Mar 27, 2018

roji commented May 14, 2018

austindrenski commented May 14, 2018

roji commented May 14, 2018

austindrenski commented May 14, 2018

roji commented May 14, 2018

austindrenski commented May 14, 2018

roji commented May 14, 2018

austindrenski commented May 14, 2018

austindrenski commented May 14, 2018

roji left a 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

austindrenski commented May 15, 2018

roji left a 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

austindrenski commented May 15, 2018

roji left a comment

Choose a reason for hiding this comment

austindrenski commented Mar 4, 2018 •

edited

Loading

Map range operations for `NpgsqlRange<T>`

`IComparable<T>`:

`range @> range = TRUE`: