Dictionary (hstore, json, and jsonb) query support by yinzara · Pull Request #3285

Dictionary (hstore, json, and jsonb) query support by yinzara · Pull Request #3285 · npgsql/efcore.pg

Conversation

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

roji mentioned this pull request

Sep 22, 2024

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.

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 changed the base branch from hotfix/9.0.4 to main

February 20, 2025 17:05

I've rebased against main. Thank you! Happy to make any other changes you'd like.

		SqlExpression? instance,
		MethodInfo method,
		IReadOnlyList<SqlExpression> arguments,
		IDiagnosticsLogger<DbLoggerCategory.Query> logger)

Choose a reason for hiding this comment

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

Here I attempted to "return null quickly" as much as possible so that you did the least amount of operations before returning null. The logic below was what I found to be the best combination while still being readable and efficient.

		/// any release. You should only use it directly in your code with extreme caution and knowing that
		/// doing so can result in application failures when updating to a new Entity Framework Core release.
		/// </summary>
		public SqlExpression? Keys(SqlExpression instance)

Choose a reason for hiding this comment

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

I choose to make these methods public and not private so they could be used externally to this translator eventually (i.e. in some expression visitor translator for more complex translations). Honestly internal probably would have been enough but I didn't see that used in a lot of places.

All public methods accept arguments of either json/jsonb or hstore arguments and return the translation or null if the arguments are not json/jsonb/hstore.

Thoughts?

		new NpgsqlDateTimeMemberTranslator(typeMappingSource, sqlExpressionFactory),
		new NpgsqlJsonDomTranslator(typeMappingSource, sqlExpressionFactory, model),
		new NpgsqlLTreeTranslator(typeMappingSource, sqlExpressionFactory, model),
		new DictionaryTranslator(typeMappingSource, sqlExpressionFactory, model),

Choose a reason for hiding this comment

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

Needs to be before the JsonPocoTranslator as it needs to intercept methods on Dictionary types that are json types.

I have now verified that all tests are passing locally after the rebase to main and all unused code and formatting changes have been removed.
I believe this is ready for a final review and approval for testing in the build system.

I know this is kind of a big PR to review but I really feel like this could be a big .NET 10 feature. Queryable dictionaries of every type seems pretty helpful!

Any chance you could get back to it? @roji

@roji It has now been 10 months from my original submission. This feature is one that has been requested by users for more than 8 years and it is fully implemented. You had said you would get to this after the .NET 9 release which was now more than 6 months ago. This feature is fully implemented IMHO and ready for merge.

Can I please get a review?

This is a great PR, I hope it will get into version 10 release. Especially mapping of Dictionary<string, string> is super useful.

@yinzara sorry that this hasn't received more attention in the previous months, I've simply been too busy (the PG provider has generally received very little attention). I admit this isn't on the top of my list generally, since hstore isn't exactly a widely-used/requested feature (the fact that something is open for X years isn't an indication that it's highly-needed, especially given it has received 6 votes total in all that time).

But in any case I'll do my best to devote some time to EFCore.PG in the coming weeks and try to get this merged for 10.