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

Consider supporting mapping HashSet (and other non-IList collections) as primitive collections #33115

Open
roji opened this issue Feb 17, 2024 · 2 comments

Comments

@roji
Copy link
Member

roji commented Feb 17, 2024

Our primitive collections feature currently supports IList<T> only, e.g. HashSet<int> doesn't work for mapping a database primitive collection. Note that EF maps primitive collections to JSON arrays (or PG arrays in PG), but these aren't sets: they are ordered and allow duplicates. For example, that means that reading a JSON array would lose data (both duplicates and the ordering).

However, it may still make sense to allow users to map the JSON/PG array, as long as the user's code uses it as a set. That is, as long as it's understood that full roundtrippability isn't supported in this case, it would work.

Note that even if we don't support mapping database columns via HashSet, we may still choose to allow supporting parameterized HashSet; this seems particularly useful for Contains queries (Where(b => someSet.Contains(b.Id))).

Note that our value comparison would need to special-case sets when comparing two primitive collections; it would likely need to call ISet.Equals if the type implements ISet (see similarity with https://xunit.net/docs/hash-sets-vs-linear-containers and xunit/xunit#2831). However, SetEquals doesn't accept a comparer, but rather uses the comparer the set was initialized with; this would make it impossible to use the element value comparer configured in EF. We'd therefore likely need to duplicate the implementation of SetEquals in our own set comparer, etc.

@m-gasser
Copy link

As far as I understand it, implementing this would fix #32365.

@roji
Copy link
Member Author

roji commented Feb 21, 2024

@m-gasser yes, the two are indeed related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants