Fix .Contains fails when the array happens to be empty

[phillip-haydon]

There's 2 tests, one that succeeds and one that fails.

The first one fails due to the unnest usage.

(query cleaned up to make it readable)

WITH mt_temp_id_list1CTE as (
    select ctid, 
              unnest(CAST(ARRAY(SELECT jsonb_array_elements_text(CAST(d.data ->> 'UserIds' as jsonb))) as integer[])) as data 
    from public.mt_doc_query_using_contains_target as d 
    WHERE CAST(d.data ->> 'IsPublic' as boolean) = :p0
), mt_temp_id_list2CTE as (
     select ctid from mt_temp_id_list1CTE where data = :p1
)
select d.id, d.data 
from public.mt_doc_query_using_contains_target as d 
where ctid in (select ctid from mt_temp_id_list2CTE)

There's a couple of issues with the generated query.

If you have a condition like

.Where(x => x.IsPublic == false || x.UserIds.Contains(10))

The first part of the query does an Unnest, if an array is Empty this will result in no rows returned for the empty array record.

The second part is filtering on the IsPublic boolean as part of the unnest.

Since the query is an OR, unless the 2 conditions are combined it wont work.

Since this is a contains the query could be simplified alot to something like:

-- when the fields are duplicated
select *
from game.mt_doc_chatchannel
where is_public is true
  or 4 = any(user_ids);

-- When the fields are not duplicated
select *
from game.mt_doc_chatchannel
where (data ->> 'isPublic')::bool is true
  or (data -> 'userIds') @> '4'::jsonb;

[mysticmind]

@phillip-haydon I have added a fix to check the value and if it is an empty array then unnest on an array with a null value. This portion of code looks a bit brittle with so many operations going on so I felt it would be safer to add a fix for this issue on the existing logic. We can definitely look at the simplification as a separate effort.

[phillip-haydon]

To be honest I don't think this will work.

If the query is an 'and' then it appears to currently work and not require a change.

If the query is an 'or' then this wouldn't work.

Using the example given. Updated with your change.

WITH mt_temp_id_list1CTE as (
    select ctid, 
              unnest(CAST(CASE WHEN {rawLocator} = '[]' THEN '{{null}}' ELSE ARRAY(SELECT jsonb_array_elements(CAST(d.data ->> 'UserIds' as jsonb))) END as jsonb[])) as data 
    from public.mt_doc_query_using_contains_target as d 
    WHERE CAST(d.data ->> 'IsPublic' as boolean) = :p0
), mt_temp_id_list2CTE as (
     select ctid from mt_temp_id_list1CTE where data = :p1
)
select d.id, d.data 
from public.mt_doc_query_using_contains_target as d 
where ctid in (select ctid from mt_temp_id_list2CTE)

It still applies the IsPublic check as part of the original query, forcing an 'and' condition.

The IsPublic check needs to be moved to the end with the in check on the array so that or can be applied.

With your change, and the moving of the condition, that would fix it, as we would need the empty array to ensure the row is returned by the cte for the where clause.

[mysticmind]

I was largely focusing on solving the issue as per the issue title trying to handle the case of array being empty. So the or condition is a different problem altogether.

[phillip-haydon]

Oh, I didn't even realise there was an exception thrown when the array was empty.

If the change fixes an exception being thrown then it should have a test specific around that otherwise we may get confused weather or not or works.

[mysticmind]

Oh, I didn't even realise there was an exception thrown when the array was empty.

If the change fixes an exception being thrown then it should have a test specific around that otherwise we may get confused weather or not or works.

This is an incorrect statement from my end i.e. "which was causing an exception", please ignore it. Corrected the earlier comment.

[mysticmind]

https://github.com/JasperFx/marten/pull/2364 looks to like a similar/same issue as this one.

[oskardudycz]

This looks good to me, although it'd be good to double-check the execution plan that's not degraded.

@jeremydmiller thoughts?

[jeremydmiller]

Hey everyone, I'll push this through tomorrow w/ all the comments if that's okay. And try to throw any new tests at it that I can.

[jeremydmiller]

Hey all, I'm sorry, but I'm going to close this PR, but open a new bug. The issue has nothing to do with arrays, it's that the SQL generated isn't respecting the "OR" condition quite right. To be honest, I don't think this is something that can be fixed with the current Linq provider until we jump to using JSONPath. The CTE expression stuff was great for SelectMany(), but it's a dead end every where else.

[jeremydmiller]

See https://github.com/JasperFx/marten/issues/2573