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

Query: Translate Enumerable.Contains() with byte arrays #4601

Closed
divega opened this issue Feb 18, 2016 · 3 comments · Fixed by #19071
Closed

Query: Translate Enumerable.Contains() with byte arrays #4601

divega opened this issue Feb 18, 2016 · 3 comments · Fixed by #19071
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@divega
Copy link
Contributor

divega commented Feb 18, 2016

We should translate Enumerable.Contains() for byte arrays, but when we do, we should be careful to update our SQL caching code:

We found while working on caching of FromSql() queries that we are special casing byte arrays so we skip structural comparisons for byte arrays (and also strings and object arrays) when comparing query cache entries. This probably because normally we treat byte arrays as a primitive type throughout our stack but for the case of using a byte array with Enumerable.Contains() each element in the byte array will become a literal in the SQL so we should consider it a different cache entry.

We should also understand if this can be a scenario for strings and object arrays.

A potential fix would be to swap to an approach that special cases Enumerable.Contains() so that we account for the fact that its first argument will not be parameterized.

cc @mikary @anpete

@divega
Copy link
Contributor Author

divega commented Feb 18, 2016

@anpete @mikary could you please check what is the impact of the bug? I wonder if subsequent queries will just reuse the SQL from the first query, returning incorrect results or if it is something more benign.

@mikary
Copy link
Contributor

mikary commented Feb 19, 2016

Attempted to produce a false positive on the caching and it appears we client eval contains with byte arrays

            using (var context = CreateContext())
            {
                var x = new byte[] { 1, 2, 3 };

                var first = context.Orders.Where(o => x.Contains<byte>((byte)o.OrderID)).ToList();

                x = new byte[] { 4, 5, 6 };

                first = context.Orders.Where(o => x.Contains<byte>((byte)o.OrderID)).ToList();
            }
SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]

SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]

The same operation with an int array produces two different cache entries:

            using (var context = CreateContext())
            {
                var x = new int[] { 1, 2, 3 };

                var first = context.Orders.Where(o => x.Contains<int>((int)o.OrderID)).ToList();

                x = new int[] { 4, 5, 6 };

                first = context.Orders.Where(o => x.Contains<int>((int)o.OrderID)).ToList();
            }
SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE [o].[OrderID] IN (1, 2, 3)

SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE [o].[OrderID] IN (4, 5, 6)

@mikary
Copy link
Contributor

mikary commented Feb 19, 2016

Opened #4605 to track caching and cache pollution aspect of this issue.
Updating this issue to address byte array client evaluation.

@mikary mikary changed the title Query: Enumerable.Contains() with byte arrays Query: Enumerable.Contains() with byte arrays forces client eval Feb 19, 2016
@rowanmiller rowanmiller changed the title Query: Enumerable.Contains() with byte arrays forces client eval Query: Translate Enumerable.Contains() with byte arrays Feb 19, 2016
@rowanmiller rowanmiller added this to the Backlog milestone Feb 19, 2016
roji added a commit that referenced this issue Nov 26, 2019
roji added a commit that referenced this issue Nov 26, 2019
@roji roji modified the milestones: Backlog, 5.0.0 Nov 26, 2019
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 26, 2019
roji added a commit that referenced this issue Nov 26, 2019
roji added a commit that referenced this issue Nov 26, 2019
@roji roji added this to the 5.0.0 milestone Nov 26, 2019
roji added a commit that referenced this issue Dec 2, 2019
roji added a commit that referenced this issue Dec 2, 2019
roji added a commit that referenced this issue Dec 3, 2019
roji added a commit that referenced this issue Dec 3, 2019
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants