-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Support non-archetypal QueryData
#21581
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
base: main
Are you sure you want to change the base?
Conversation
I ran a benchmark (on laptop):
Seems to be within noise.
But i don't know how to interpret the results; an LLM said that the Option was indeed optimized away |
Previous approval was a mistake, but I don't know how to remove it.. |
Co-authored-by: Periwink <[email protected]>
Also fix an edge case for transmutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved contingent on #17647 is the direction we want to go in. I haven't reviewed that pr yet, so I have no opinion on that yet. The changes here are pretty straightforward.
Objective
For #17647, we want to create a
QueryData
that can follow a relation and query data from an entity's parent. If the parent does not have the queried data, the child entity should be skipped in the query. However, there is no way to tell from the child's archetype whether the parent will match! So, we need to support non-archetypalQueryData
, just as we support non-archetypalQueryFilter
s forAdded
andChanged
.That is, if
Query<Parent<&T>>
yields&T
, and we do:then
query
must yield a row forchild1
but not forchild2
, even though they have the same archetype.Solution
Change
QueryData::fetch
to returnOption
so that entities can be filtered during fetching by returningNone
.To support
ExactSizeIterator
, introduce anArchetypeQueryData
trait and anQueryData::IS_ARCHETYPAL
associated constant, similar toArchetypeFilter
andQueryFilter::IS_ARCHETYPAL
. Implement this trait on existingQueryData
types. ModifyExactSizeIterator
implementations to requireD: ArchetypeQueryData
, and thesize_hint()
methods to return a minimum size of0
if!D::IS_ARCHETYPAL
.Alternatives
We could do nothing here, and have
Query<Parent<&T>>
yieldOption<&T>
. That makes the API less convenient, though. Note that if one wants to query forOption
, they can use eitherQuery<Option<Parent<&T>>
orQuery<Parent<Option<&T>>
, depending on whether they want to include entities with no parent.Another option is to re-use the
ArchetypeFilter
trait instead of introducing a new one. There are no places where we want to abstract over both, however, and it would require writing bounds likeD: QueryData + ArchetypeFilter, F: QueryFilter + ArchetypeFilter
instead of simplyD: ArchetypeQueryData, F: ArchetypeFilter
.