Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to 3.0.0-preview7 #943

Merged
merged 13 commits into from
Jul 25, 2019
Merged

Port to 3.0.0-preview7 #943

merged 13 commits into from
Jul 25, 2019

Conversation

roji
Copy link
Member

@roji roji commented Jul 24, 2019

This (finally) ports EFCore.PG to work against upstream preview7, notably the new query pipeline.

This PR isn't 100% complete but I have to leave the office and I thought I'd give people a chance to take a look. There's probably some residual messiness.

I don't know if I can realistically expect a thorough review of such a big PR, but @austindrenski @YohDeadfall at least a cursory look for glaring issues could definitely help. If at all possible I'd like to release this tomorrow or Friday.

/cc @divega @ajcvickers @smitpatel in case you want to take a peak.

Copy link

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Skimmed through query changes. Most look fine.
I see some of custom expression are using string where they can use ExpressionType enum. Though your preference.
Quite a few places you have applied default type mapping, can skip if you are using factory as factory should do the same for you.

Copy link
Contributor

@austindrenski austindrenski left a comment

Choose a reason for hiding this comment

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

Partial:

  • Directory.Build.props
  • ...
  • src/EFCore.PG/Extensions/MethodInfoExtensions.cs
  • ...
  • test/EFCore.PG.FunctionalTests/Query/SimpleQueryNpgsqlTest.Where.cs
  • ...
  • tools/Resources.tt

@@ -323,11 +323,11 @@ public void ValueComparer_hstore()

[Fact]
public void GenerateSqlLiteral_returns_jsonb_literal()
=> Assert.Equal(@"JSONB '{""a"":1}'", GetMapping("jsonb").GenerateSqlLiteral(@"{""a"":1}"));
=> Assert.Equal(@"'{""a"":1}'", GetMapping("jsonb").GenerateSqlLiteral(@"{""a"":1}"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is something else responsible for generating the prefix now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question :)

One of the great new features of the 3.0 query pipeline is type inference. Previously, EF Core used to find the default type mapping for a given CLR type, and use that to generate both literals and parameters. This is what caused all the issues we saw with citext, as a citext column was compared with a string parameter, and we'd send the parameter as text and PostgreSQL would perform case-sensitive comparison.

With the new pipeline, the type mappings of parameters and literals are inferred from their context: if you compare a citext column to a string literal or parameter, the column's type mapping (citext) will be applied to the other side, and the parameter (and literal) will be generated correctly. That's a great improvement.

Now after some thought, I'm pretty sure that in most cases we don't actually need typed literals, as PostgreSQL will infer their types based on context as well. This isn't related to the new EF inference feature - it just made me think about this again. Do you see any scenario where JSONB has to be explicitly specified in order for PostgreSQL to correctly understand expression?

/cc @smitpatel

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you see any scenario where JSONB has to be explicitly specified in order for PostgreSQL to correctly understand expression?

My initial thoughts were about JSON vs JSONB inference, and how that might impact function/operator resolution (and maybe trivial round-tripping?).

But perhaps the new pipeline still delegates to the provider for type resolution and/or casting when it comes to custom operators?

austindrenski=> SELECT JSONB '{"a": {"b":{"c": "foo"}}}' #> '{a,b}';

   ?column?   
--------------
 {"c": "foo"}
(1 row)
austindrenski=> SELECT '{"a": {"b":{"c": "foo"}}}' #> '{a,b}';

ERROR:  operator is not unique: unknown #> unknown
LINE 1: select '{"a": {"b":{"c": "foo"}}}' #> '{a,b}';
                                           ^
HINT:  Could not choose a best candidate operator. You might need to add explicit type casts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good discussion - but it helps to think first about the C# LINQ side which would produce the SQL. Specifically, how would you go about producing the SQL above with LINQ? The C# counterpart to jsonb is a string, but a string literal by default gets mapped to text, not jsonb.

More generally, the SQL you wrote above involves two literals - that's quite an artificial scenario that may not always be fully supported. The generic logic with inference is that somewhere in your expression we're going to have a column, which has a clear and definite type mapping (json, jsonb, citext, whatever). As that column is combined in expressions (binary, functions, whatever), its type mapping is applied to the other side(s) of the expression, and so the type mapping bubbles up as necessary. In your example there's only literals, so no way of knowing about any specific type mappings.

@smitpatel may have additional thoughts on this. Do you think there's a good reason to provide explicit typing for literals of type mappings which aren't the default for their CLR type?

@roji
Copy link
Member Author

roji commented Jul 24, 2019

@smitpatel thanks for reviewing!

I see some of custom expression are using string where they can use ExpressionType enum. Though your preference.

I'll take a look. It may make sense in some cases such as ArrayAnyAllExpression, which can in theory output any PostgreSQL operator.

Quite a few places you have applied default type mapping, can skip if you are using factory as factory should do the same for you.

Yeah, my understanding of the new inference system and expression factory has evolved as I worked through this. I'll try to do a pass to remove unnecessary stuff.

austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request Jul 25, 2019
This may cause conflicts with npgsql#943
austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request Jul 25, 2019
This may cause conflicts with npgsql#943
austindrenski added a commit to austindrenski/npgsql-efcore.pg that referenced this pull request Jul 25, 2019
This may cause conflicts with npgsql#943
@roji
Copy link
Member Author

roji commented Jul 25, 2019

Rebased after #929 and did some last fixed, this should hopefully be mergeable now.

@roji roji merged commit c4df1a9 into npgsql:dev Jul 25, 2019
@roji roji deleted the preview7 branch July 25, 2019 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants