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

POC of Mql.Field (could be used by EF Core Provider to support shadow properties). #1373

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rstam
Copy link
Contributor

@rstam rstam commented Jun 27, 2024

No description provided.

@rstam rstam requested a review from a team as a code owner June 27, 2024 22:39
var collection = GetCollection<C>();

var queryable = collection.AsQueryable()
.Select(root => Mql.Field(root, "X", Int32Serializer.Instance) + 1); // like root.X except C does not have a property called X
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to know the container, even if it is only the root.

X is like a shadow property. It exists in the document in the database but not in the class C.

@rstam rstam removed the request for review from a team June 27, 2024 22:48
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

I like the overall technique. If we are going to productionize it, we probably would want to make the serializer optional, which poses some challenges. root => Mql.Field<int>(root, "X") + 1 feels more natural, but C# doesn't allow partial generic type inferencing.

I can see where specifying the serializer makes sense for advanced use cases, but in common cases such as strings, integers, bools, etc., it will be more useable to specify the field type.

@rstam
Copy link
Contributor Author

rstam commented Jul 17, 2024

The serializer is important because it defines how the field is represented in the database.

If it is not provided, where would we get it from? The only reasonable place would be the BsonSerializerRegistry, but there is no guarantee that the serializer registered there is the correct one for this particular hidden field in this particular POCO.

@damieng
Copy link
Member

damieng commented Nov 4, 2024

Tested and working well with the EF Shadow Properties implementation and also for select projection rewriting.

@damieng
Copy link
Member

damieng commented Nov 14, 2024

Two open questions here:

  1. Should the string "fieldName" second parameter of Mql.Field be elementName or even elementPath?
  2. When using Mql.Field I'm seeing:
{ "$match": { 
   "$and": [
      { "$expr" : { "$eq" : ["$planetId", { "$oid" : "621ff30d2a3e781873fcb663" }] } },
      { "$expr" : { "$eq" : ["$label", "VI"] } }
   ] 
},
{ "$limit" : 1 }

But when executing against the same properties as CLR properties I get a quite different query:

  { "$match" :
  	{ "_id.planetId" : { "$oid" : "621ff30d2a3e781873fcb663" },
  	{ "_id.label" : "VI" } 
  }, 
  { "$limit" : 1 }

Why are the ones from MqlFied using $expr and why do they get wrapped in an additional $and ?

@rstam
Copy link
Contributor Author

rstam commented Nov 15, 2024

Why are the ones from MqlFied using $expr and why do they get wrapped in an additional $and ?

Because initially I only implemented Mql.Field for aggregation expressions and not for filters.

The $and is required because implicit and can't be used when there are two $expr in the same document (that would be duplicate field names).

@rstam
Copy link
Contributor Author

rstam commented Nov 15, 2024

Should the string "fieldName" second parameter of Mql.Field be elementName or even elementPath?

It's a bit arbitrary when we use "field" and when we use "element".

I don't think the server team ever uses the term "element". To them it's always a "field".

@rstam
Copy link
Contributor Author

rstam commented Nov 15, 2024

or even elementPath?

Not sure about paths... I was envisioning that Mql.Field would descend EXACTLY ONE level. If there are more levels you can compose multiple Mql.Field uses to descend to the nested field.

@rstam
Copy link
Contributor Author

rstam commented Nov 15, 2024

or even elementPath?

Looking at this in more detail I believe that Mql.Field can only descend one level.

Mql.Field translates to the MQL $getField operator which only descends one level at a time.

The server expects you to nest calls to $getField to descend multiple levels.

@rstam
Copy link
Contributor Author

rstam commented Nov 15, 2024

I've pushed a new commit adding support for Mql.Field in filters.

@damieng
Copy link
Member

damieng commented Nov 21, 2024

POC working great here now as the backing for EF-42 (shadow properties).

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.

3 participants