-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
New EF.Functions.Collate #20603
New EF.Functions.Collate #20603
Conversation
9de5bd5
to
ea7adcf
Compare
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
@smitpatel addressed review comment. |
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
Sorry about the previous nonsense, hopefully I got it right this time. |
@roji be like |
{ | ||
Check.NotNull(collateExpresion, nameof(collateExpresion)); | ||
|
||
return collateExpresion.Update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maumar to verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maumar @smitpatel are we waiting for anything in particular here? Would be good to merge this for preview4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is correct - signed off
test/EFCore.Relational.Specification.Tests/Query/RelationalNorthwindDbFunctionsQueryTestBase.cs
Outdated
Show resolved
Hide resolved
Made abstract, let me know if you have any more comments. Today @roji be more like: |
@roji @smitpatel I haven't read through the comments here but I noticed there are a lot! Is there anything in particular I should take a look at or needs resolving? |
@ajcvickers I don't think so, from my side everything is fine - just waiting for approval. |
@smitpatel new version up. |
Rebased and made collation abstract in migrations tests too (#20646) |
|
||
namespace Microsoft.EntityFrameworkCore.Query.Internal | ||
{ | ||
public class CollateTranslator : IMethodCallTranslator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add internal docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem to be missing from all translators, will submit a separate PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore. I already did this as part of query docs work. I am trying to make sure we have docs for all new stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks!
src/EFCore.Relational/Query/SqlExpressions/CollateExpression.cs
Outdated
Show resolved
Hide resolved
async, | ||
ss => ss.Set<Customer>(), | ||
ss => ss.Set<Customer>(), | ||
c => EF.Functions.Collate(c.ContactName, CaseSensitiveCollation) == "maria anders", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the effect of using EF.Functions.Collate on a string column and projecting it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming I understand your question: the collation aspect bubbles up across projections - does this look good?
SQL Server:
DROP TABLE data;
CREATE TABLE data (
id INT IDENTITY,
cs VARCHAR(MAX) COLLATE Latin1_General_CS_AS
);
INSERT INTO data (cs) VALUES ('foo');
SELECT * FROM data WHERE cs = 'FOO'; -- no results because column is case-sensitive
SELECT * FROM (
SELECT cs COLLATE Latin1_General_CI_AS AS cs
FROM data
) AS t
WHERE t.cs = 'FOO'; -- returns row because cs projects out with the explicitly-applied collation
PostgreSQL:
CREATE COLLATION cs_collation (LC_COLLATE = 'en-u-ks-primary',
LC_CTYPE = 'en-u-ks-primary',
PROVIDER = icu,
DETERMINISTIC = False
);
DROP TABLE IF EXISTS data;
CREATE TABLE data (
id INT GENERATED ALWAYS AS IDENTITY,
cs TEXT COLLATE "POSIX"
);
INSERT INTO data (cs) VALUES ('foo');
SELECT * FROM data WHERE cs = 'FOO'; -- no results because column is case-sensitive
SELECT * FROM (
SELECT cs COLLATE cs_collation AS no_longer_cs
FROM data
) AS t
WHERE t.no_longer_cs = 'FOO'; -- returns row because cs projects out with the explicitly-applied collation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nooo. I wanted a test where you are using EF.Collate in projection and if it generates any difference in reading results. (which it should not.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh you mean top-most projection (i.e. on a column that will be read by the client)? If so I'll check that, no problem (did you mean you want actual test code for this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the following (which passes on both providers):
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Collate_column_projection(bool async)
=> AssertQuery(
async,
ss => ss.Set<Customer>().Select(c => EF.Functions.Collate(c.ContactName, CaseInsensitiveCollation)),
ss => ss.Set<Customer>().Select(c => c.ContactName));
If so do you want this in the test suite?
Closes #8813
@smitpatel @ajcvickers what's your opinion on merging this into the preview4 branch? It would be good to deliver this with the other collation functionality (column/database migration support) |
@roji I think it's probably okay just to wait. |
Closes #8813