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

Dictionary (hstore, json, and jsonb) query support #3285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yinzara
Copy link

@yinzara yinzara commented Sep 19, 2024

This PR adds support for querying on hstore type columns which can map to either Dictionary<string, string> or ImmutableDictionary<string, string> in C# and for querying any other Dictionary<,> or ImmutableDictionary<,> for json and jsonb types.

See npgsql/doc#369 for full documentation.

Fixes #212

@yinzara yinzara force-pushed the feature/212 branch 2 times, most recently from 8f6dad8 to 49431a8 Compare September 19, 2024 19:21
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks @yinzara, looks pretty good overall! See comments below.

@roji
Copy link
Member

roji commented Sep 22, 2024

I attempted Keys and Values and could never get it to work correctly.

If you translate Keys via akeys, that simply returns an array, so shouldn't be complicated (note that the provider will automatically call unnest on that array if you start composing arbitrary operators on it). If you translate via skeys which returns a set, things are definitely more complicated.

@yinzara yinzara force-pushed the feature/212 branch 21 times, most recently from df2adf1 to b4e4966 Compare October 4, 2024 07:15
@yinzara
Copy link
Author

yinzara commented Oct 4, 2024

Apologies for the mountains of force pushes :-) I just kept realizing there was more I could do or make better :-)

I think I'm done now lol all tests pass and everything I could possibly translate I have translated.
Every operator in:
https://www.postgresql.org/docs/current/hstore.html
that has some kind of C# equivalent is now covered.

I would have loved to have supported:

string[] keys = ["key1", "key2"];

_ => _.Store.Keys.Any(k => keys.Contains(k)) 
which would become
"Store" ?|  __keys

and the equivalent .All => ?&

however that simply isn't possible using the IMethodCallTranslator or a IMemberTranslator

FINALLY
ready for review @roji

@yinzara
Copy link
Author

yinzara commented Oct 4, 2024

I started to write up the changes to the npgsql/doc site for hstore and realized that I could use EF.Functions to get the rest of the supported operators for hstore so that we can have the implementation completely done.

I've still got a few more test cases to write, but the new and improved hstore functionality is now complete. See the documentation PR at:
npgsql/doc#369

For more details.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

@yinzara thanks for making these changes and for the enthusiasm.

First, in any case where there's a straightforward LINQ construct that can be used to express something, that's preferred over introducing a method on EF.Functions. For example, something like ContainsAnyKeys can be expressed in LINQ via dict.Keys.Contains(foo), so I wouldn't introduce EF.Functions.ContainsAnyKeys(). It's indeed a bit more complicated to do translations like the former (not possible in a method translator), but I think it's fine to leave them out for the time being.

Second, I appreciate the work, but I'd rather we didn't eagerly introduced translations for each and every possible thing that PostgreSQL supports, but rather base the work on what users actually express a need for. The hstore type generally is rarely used, and crucially, has been made somewhat obsolete by PG's json support - JSON can do pretty much everything hstore can, and more. Remember that at the end of the day, I have to maintain all this stuff going forward...!

So I'd be happier if at least as a first step, we added the basic, simple stuff which users would be more likely to use, and then wait for people to actually ask for more features before implementing them.

Hope that makes sense.

@yinzara
Copy link
Author

yinzara commented Oct 8, 2024

So it turns out this PR and its related issue is highly related to

dotnet/efcore#29825
And
dotnet/efcore#26903

I think that I can actually change this PR so that it is focused on that instead and just happens to work with 'hstore' types as well that way this ends up being something people want as a whole. That would mean I'd refocus the PR on translating any Dictionary or ImmutableDictionary query no matter if it's hstore, json or jsonb

@yinzara yinzara changed the title Hstore query support Dictionary (hstore, json, and jsonb) query support Oct 9, 2024
@yinzara yinzara force-pushed the feature/212 branch 13 times, most recently from 50c95a9 to 961a7df Compare October 11, 2024 09:13
@yinzara
Copy link
Author

yinzara commented Oct 11, 2024

@roji
I think I've got another incarnation that's worth your eyes to at least give some guidance if you think I'm headed in the right direction. I still have a few more tests to write if you agree.

This PR is now focused 100% on all forms of Dictionary<string,TValue> and ImmutableDictionary<string,TValue> based querying no matter if it's hstore, json or jsonb no matter their TValue types.

I removed all of the DBFunction extensions that could ever have a translation that would be supported without the DBFunctions. A future PR (or maybe if I get ansy) can address doing the expression visitor changes to support the other queries.

I now have a test case with a Dictionary<string, Dictionary<string, string>> NestedDictionary {get;set;} that shows how the functionality even works recursively :-) and another test case with Dictionary<string, int> to show it works with numbers as well.

Finally I made the new DictionaryTranslator not look up any TypeMappings inside the constructor so it's very efficient to create new instances of it with no overhead.

This does not fix dotnet/efcore#26903 as I was thinking it might, but it still seems quite useful overall and eventually it could support it.

@yinzara
Copy link
Author

yinzara commented Nov 22, 2024

I know it's been a while since you've had time to look at this. Any chance you could take a peak and at least tell me if you are ok with my direction now? If so I'll update all the test cases and make sure it's ready for merge.

@@ -135,6 +135,7 @@ protected override void Print(ExpressionPrinter expressionPrinter)
PgExpressionType.JsonExists => "?",
PgExpressionType.JsonExistsAny => "?|",
PgExpressionType.JsonExistsAll => "?&",
PgExpressionType.JsonValueForKeyAsText => "->>",
Copy link
Author

Choose a reason for hiding this comment

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

I realize this isn't necessary now and I'll make sure it's removed before the PR merges.

@roji
Copy link
Member

roji commented Nov 23, 2024

@yinzara I'll do my best to look soon, but it's a very busy time in the post 9.0 phase; so it may take a little while. Don't hesitate to ping me again in a month or two if you see no progress here.

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.

Translate hstore access to SQL
3 participants