Added basic support for HStore operations query translation #240
Conversation
|
@dejvid-smth just a quick message to say thanks for this PR. I'm currently too busy to properly review it but will do so as soon as I can... Good question regarding the equality comparison for changing tracking in EF Core - I'm not sure how things work. If this is an issue, it would also be an issue for other types already supported by Npgsql (e.g. arrays). I would expect EF Core to simply execute Am not sure that an ImmutableDictionary is a good solution for this - forcing the user to create an entire copy of the dictionary each time a change is needed is quite a poor substitute for proper change tracking. |
|
Created issue regarding change tracker not working for Dictionary<,> |
|
@dejvid-smth note that EF Core has added support for custom comparison which would solve the change tracking issue mentioned here. I opened #306 to track this. |
|
|
||
| public Expression Translate(MethodCallExpression methodCallExpression) | ||
| { | ||
| if (methodCallExpression.NodeType == ExpressionType.Call |
There was a problem hiding this comment.
MethodCallExpression is always ET.Call? Redundant check.
| public Expression Translate(MethodCallExpression methodCallExpression) | ||
| { | ||
| if (methodCallExpression.NodeType == ExpressionType.Call | ||
| && methodCallExpression.Method.Name == "get_Item" |
There was a problem hiding this comment.
Should be easy to compare with actual MethodInfo rather than string name.
| public Expression Translate(MethodCallExpression methodCallExpression) | ||
| { | ||
| if(methodCallExpression.Method == | ||
| typeof(NpgsqlDbFunctionsExtensions) |
There was a problem hiding this comment.
store in static field to avoid doing reflection for every translation.
Same for the other If
| @@ -176,6 +192,15 @@ public Expression VisitArrayAny(ArrayAnyExpression arrayAnyExpression) | |||
| return arrayAnyExpression; | |||
| } | |||
|
|
|||
| public Expression VisitDictionaryContainsKey(DictionaryContainsKeyExpression expr) | |||
| { | |||
| Visit(expr.Dictionary); | |||
There was a problem hiding this comment.
This could be written using SqlFunctionExpression in 2.1 passing in dictionary as instance. That would avoid creating new expression type
| && typeof(IDictionary<string, string>).IsAssignableFrom(expression.Object.Type)) | ||
| { | ||
| Debug.Assert(expression.Arguments.Count == 1); | ||
| var expr = Visit(expression.Object); |
There was a problem hiding this comment.
Same as the other. Can be SqlFunctionExpression.
|
Thanks for the review @smitpatel! @dejvid-smth, with dotnet/efcore#9753 closed on the EF Core side, it's possible to implement proper change tracking for hstore (#306). Are you interested in continuing work on this issue for the upcoming 2.1 release? If so, please let me know and take @smitpatel's comments into account. |
|
I am currently busy with my thesis but I can get back to it in 3 months if no-one picks it up |
roji
force-pushed
the
dev
branch
3 times, most recently
from
f320396 to
dfdc336
Compare
|
Closing this PR as stale... Many things have changed since this was submitted (notably the query pipeline rewrite for 3.0), so this would likely have to be redone. |
I looked into how hstore support could be implemented. Some basic operations work but it is still not good enough, in my opinion, to be used properly.
When dictionary object that represents hsore column is updated it is not tracked by EF's change tracker so users would have to reassign previous value with new dictionary before every call to
SaveChanges()which is not really a safe solution in my opinion.I tried using ImmutableDictionary which would solve this problem but the serializer does not support ImmutableDictionary so we can not deserialize into it after we get data from DB.
So