Open
Conversation
cdc-as81
changed the title
Benchmark ResultsHyperfine
CriterionRegressions (slower)
Improvements (faster)
Unchanged / inconclusive (CI crosses 0%)
|
RobertJacobsonCDC
requested changes
Mar 31, 2026
Collaborator
RobertJacobsonCDC
left a comment
RobertJacobsonCDC
left a comment
There was a problem hiding this comment.
This PR changes all examples and tests to use the with! macro consistently for
Queryinstances (query_entity_count,with_query_results,match_entity,sample_entity)PropertyListinstances (add_entity)
It also introduces the private trait QueryTuple, which mirrors Query, though it's not clear what its purpose is. QueryTuple is implemented for tuples up to size 20.
What this PR doesn't do is remove tuple queries. Query is still implemented for () and for tuples of properties up to length 20. I think the intention of the issue was that we'd remove the Query impl for all tuples and require client code to use the with! macro (and thus EntityPropertyTuple).
Because you already have a blanket impl<E: Entity, T: QueryTuple<E>> Query<E> for EntityPropertyTuple<E, T>, you can achieve this by just removing the implementation
impl<
E: Entity,
#(
T~N : Property<E>,
)*
> Query<E> for (
#(
T~N,
)*
) {...}from the impl_query! macro definition. Then QueryTuple<E> would make sense: it allows EntityPropertyTuple<E, T> to implement Query without the tuple T implementing Query.
We have a blanket implementation of PropertyList for EntityPropertyTuple<E, T> whenever T: PropertyList. We could also require users to use the with! macro for property lists (adding new entities), in which case we would remove the implementation of PropertyList for tuples up to size 20 as well. We could use the same hypothetical mechanism as for tuples: introduce a private PropertyListTuple trait that mirrors PropertyList and provide a blanket impl<E: Entity, T: PropertyListTuple<E>> PropertyList<E> for EntityPropertyTuple<E, T> that delegates to PropertyListTuple<E>.
Benchmark ResultsHyperfine
CriterionRegressions (slower)
Improvements (faster)
Unchanged / inconclusive (CI crosses 0%)
|
Benchmark ResultsHyperfine
CriterionRegressions (slower)
Improvements (faster)
Unchanged / inconclusive (CI crosses 0%)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces entity-scoped query/property-bundle construction with
with!(Entity, ...)andEntitywhile preserving compatibility with the legacy tuple-based query API.What Changed
with!(Entity, ...)as the preferred public constructor for:add_entity(...)EntityPropertyTuple<E, T>as the public wrapper for entity-scoped property bundlesQueryTuple<E>trait inquery_impls.rsQuery::is_empty_query()so whole-population queries can be handled explicitlyQuery<E>andPropertyList<E>so callers can usePerson/with!(Person)for whole-population operationsis_empty_query()instead of special-casing()Backward Compatibility
Legacy query forms are still supported:
(Age(42),)and(Age(42), RiskCategory::High)continue to work()are still supported where type inference is available or the entity type is specified explicitlyThis keeps older code working while allowing newer call sites to use the clearer
with!(Entity, ...)form.Why
The previous API relied heavily on raw tuples and
()for queries, which made entity scope implicit. This change makes the public API more explicit and readable:with!(Person, Age(42))is clearly aPersonquerywith!(Person)andPersonclearly represent whole-population queriesTesting
Added and updated tests to cover:
with!(Entity)empty-query behaviorwith!(Entity, ...)single- and multi-property queriesEntityPropertyTuple