Translate simple range operators + compatibility support #616

austindrenski · 2018-08-23T18:03:36Z

Closes: #609

+        /// Constructs a new instance of the <see cref="NpgsqlRangeTranslator"/> class.
+        /// </summary>
+        /// <param name="postgresVersion">The backend version to target.</param>
+        public NpgsqlRangeTranslator([CanBeNull] Version postgresVersion) => _postgresVersion = postgresVersion;


Move the assignment one line down.

YohDeadfall · 2018-08-23T19:14:47Z

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

+            {
+            case nameof(NpgsqlRange<int>.LowerBound) when VersionAtLeast(9, 2) && !e.Type.IsNullableType():
+                return
+                    new SqlFunctionExpression(


Put it on the same line as return and decrease indent for lines below.

YohDeadfall · 2018-08-23T19:16:21Z

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

+
+            case nameof(NpgsqlRange<int>.UpperBound) when VersionAtLeast(9, 2) && !e.Type.IsNullableType():
+                return
+                    new SqlFunctionExpression(


YohDeadfall · 2018-08-23T19:19:26Z

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

+        /// <returns>
+        /// True if <see cref="_postgresVersion"/> is null, greater than, or equal to the specified version.
+        /// </returns>
+        bool VersionAtLeast(int major, int minor)


Documentation is a cool thing, but not in such situations (: It's an private one liner with very simple logic.

I totally agree :)

Fair fair...

Actually, I was thinking about pulling this out into an internal extension class (maybe into Check?) since it's used in a couple of different files now. What do you two think?

I think it's OK for something this small to be duplicated...

YohDeadfall · 2018-08-23T20:14:09Z

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

+
+            string GenerateLiteralFromMapping(Type t)
+                => TypeMappingSource.FindMapping(t)
+                                    .GenerateSqlLiteral(t.IsNullableType() ? null : Activator.CreateInstance(t));


Why not a lambda expression?

Having this inline led to either a really long line....or some odd looking indentation (even by my standards :)).

I thought this looked a tiny bit more readable, but just barely. I don't have a strong opinion on this one, so I can refactor if you think the lambda is more readable.

What do you think about the following code?

Sql.Append(DefaultConstants.GetOrAdd(e.Type, t=> TypeMappingSource.FindMapping(t).GenerateSqlLiteral( t.IsNullableType() ? null : Activator.CreateInstance(t)));

YohDeadfall · 2018-08-23T20:16:01Z

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

+            {
+                try
+                {
+                    var _ = context.RangeTestEntities.Select(x => x.Range.LowerBound).ToArray();


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

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.

Okay, let's leave it for Rider.

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

Actually scratch that, your way is better :)

roji · 2018-08-24T04:21:13Z

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

            {
-            case nameof(NpgsqlRangeExtensions.Contains):
-                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "@>", typeof(bool));
+            case nameof(NpgsqlRangeExtensions.Contains) when VersionAtLeast(9, 2):


I'm not sure the range translator should actually be version-aware, and avoid to translate in pre-9.2. The entire range type doesn't exist prior to 9.2, any attempt to create a range column will fail. It seems better to let EF Core send range SQL, and have PostgreSQL fail it with a PostgreSQL error, rather than get NotSupportedException from client-evaluation.

As a general rule we try not to do a client-side check where a clean, clear database-side check already exists. In other cases we do refrain from translating version-unsupported exceptions since client evaluation works (e.g. DateTime.Add*()), but in this case there's no valid client evaluation.

That's a good point. I was thinking about someone using the NpgsqlRange<T> as a CLR type not intended for PostgreSQL consumption....but that's not something we would encourage anyways, so throwing does sounds better.

(Note this logic applies to the new member translations where they can be client evaluated. Adding the checks to the extension methods was just some overzealous copy/pasting.)

roji · 2018-08-24T04:21:50Z

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

-                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) when VersionAtLeast(9, 5):


Same comment as above. I'd rather the user found out about the lack of range_merge() from a PostgreSQL error.

roji · 2018-08-24T04:25:11Z

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

+
+            switch (e.Member.Name)
+            {
+            case nameof(NpgsqlRange<int>.LowerBound) when VersionAtLeast(9, 2) && !e.Type.IsNullableType():


I'd not have two cases for value/reference, but handle them in the same case. You can even create the SqlFunctionExpression once and conditionally wrap it based on whether the type is nullable.

Actually, thinking about this some more, is this really the desired behavior? In which cases does PostgreSQL return null for a bound? Do we really want them to zeros when it's an int range?

That's a good point, I'll merge these cases.

More details below, but the short answer is that we can't return a database null as the bound for a range over CLR value types. So Range<T>.LowerBound needs a coalesce wrapper if T is non-nullable, because the range could be empty or the bound could be infinite.

I played around with a few ways to mitigate this, and this seemed to be the least hack-ish. That said, it's entirely possible that I've overlooked a simpler solution, so I'm open to any alternatives. (It would be nice not to need the coalesce wrapper...)

I'll try to investigate a bit tomorrow.

roji · 2018-08-24T04:26:02Z

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

+        /// <returns>
+        /// True if <see cref="_postgresVersion"/> is null, greater than, or equal to the specified version.
+        /// </returns>
+        bool VersionAtLeast(int major, int minor)


I totally agree :)

roji · 2018-08-24T04:29:27Z

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

@@ -198,6 +209,17 @@ protected override Expression VisitBinary(BinaryExpression expression)
            }
        }

+        /// <inheritdoc />
+        protected override Expression VisitDefault(DefaultExpression e)


What is this required for exactly?

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.

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.

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

roji

Sorry for forgetting about this, it's been a very busy few weeks recently...

I looked again and everything seems to make sense. I'd include a comment clarifying why VisitDefault() necessary, pointing specifically to the range translator which is creating the default expression.

I'm also not sure it's worth having a ConcurrentDictionary to cache default literals (the entire tree is cached at the EF Core level) but it's not big deal in any way.

Translate simple range operators + compatibility support

2db1fdb

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

austindrenski self-assigned this Aug 23, 2018

austindrenski requested review from roji and YohDeadfall August 23, 2018 18:03

YohDeadfall suggested changes Aug 23, 2018

View reviewed changes

roji requested changes Aug 24, 2018

View reviewed changes

roji reviewed Aug 24, 2018

View reviewed changes

Address review

0573c05

YohDeadfall approved these changes Aug 24, 2018

View reviewed changes

roji approved these changes Sep 23, 2018

View reviewed changes

Add comment and simplify visitor

44c2f14

austindrenski merged commit edf6e3f into npgsql:dev Sep 23, 2018

austindrenski deleted the simple-range-operators branch September 23, 2018 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate simple range operators + compatibility support #616

Translate simple range operators + compatibility support #616

austindrenski commented Aug 23, 2018

YohDeadfall Aug 23, 2018

YohDeadfall Aug 23, 2018

YohDeadfall Aug 23, 2018

YohDeadfall Aug 23, 2018

YohDeadfall Aug 23, 2018

roji Aug 24, 2018

austindrenski Aug 24, 2018

roji Aug 25, 2018

YohDeadfall Aug 23, 2018

austindrenski Aug 24, 2018

YohDeadfall Aug 24, 2018

YohDeadfall Aug 23, 2018

austindrenski Aug 24, 2018 •

edited

Loading

YohDeadfall Aug 24, 2018

roji Aug 25, 2018 •

edited

Loading

roji Aug 25, 2018

roji Aug 24, 2018

austindrenski Aug 24, 2018 •

edited

Loading

roji Aug 24, 2018

roji Aug 24, 2018

austindrenski Aug 24, 2018

roji Aug 25, 2018

roji Aug 24, 2018

roji Aug 24, 2018

austindrenski Aug 24, 2018

roji Aug 25, 2018

austindrenski Sep 20, 2018

roji left a comment

+                              new IMemberTranslator[]
+                              {
+                                  new NpgsqlRangeTranslator(npgsqlOptions.PostgresVersion)
+                              };

Translate simple range operators + compatibility support #616

Translate simple range operators + compatibility support #616

Conversation

austindrenski commented Aug 23, 2018

Related

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

austindrenski Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austindrenski Aug 24, 2018 • edited Loading

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

Choose a reason for hiding this comment

austindrenski Aug 24, 2018 •

edited

Loading

roji Aug 25, 2018 •

edited

Loading

austindrenski Aug 24, 2018 •

edited

Loading