chore: Index multi-properties by default

[cdc-as81]

Summary

• Make define_multi_property! create a FullIndex by default via
  Property::default_index_type.
• Allow a multi-property definition to opt into ValueCountIndex or opt out
  with PropertyIndexType::Unindexed.
• Preserve feat: Replace universal Property: Eq + Hash constraint with IndexableProperty #937’s no-shared-storage design: equivalent multi-properties remain
  distinct, while the first registered representative handles query routing and
  is the only one eligible for a default index.
• Keep explicit Context::index_property calls compatible with populated
  multi-properties, including default-indexed and unindexed definitions.
• Update documentation and regression coverage for defaults, overrides,
  equivalent property ordering, and explicit indexing.

[RobertJacobsonCDC]

I think this needs to be blocked by #937. Also, it's not clear which kind of index should be created. We should discuss how this interacts with these issues, too:

• Optimistically create indexes or warn #280

• Profiling infrastructure for indexes #283

• Add query tracking to profiler #705

[cdc-as81]

lets add an option to the macro to set the index type.
property_store.set_property_indexed::

(PropertyIndexType::FullIndex);

[RobertJacobsonCDC]

PR #937 removed sharing indexes between properties. This PR appears to reintroduce shared properties. I don't think we want to do this. Instead, let's retain the warning and error behavior explicitly introduced in #937. I think this will simplify this PR quite a bit.

There's also the philosophical question of whether we want to expose Property::default_index_type as public API. So for example, we could add a parameter to define_*_property! / impl_*_property that lets client code set the return value of Property::default_index_type. But let's defer that question to another issue / PR. (I don't think we have a use case for it, so let's not pay the cost of added code complexity.)

[cdc-as81]

Fixed. Equivalent multi-properties now use distinct storage; only the registered representative receives the default index.

[RobertJacobsonCDC]

This PR is a lot more invasive than I expected, and I can't tell if it is because I don't fully understand some edge cases or if the complexity is genuinely unnecessary.

The PR adds:

• The single method add_to_property_registry is now expanded to add_indexed_to_property_registry and add_to_property_registry_with_constructor.
• PropertyValueStoreCore gets a new constructor new_boxed_with_default_index.
• PropertyMetadata gets a new field index_new_entity_fn

The index_new_entity_fn field is the most suspicious to me. When a new property is registered (before main executes), it stores a constructor in value_store_constructor that can construct a new property value store for the property in a type erased way. This constructor is used in the constructor of a PropertyStore<E> (the thing that holds every property value store for every property of an entity) so that the PropertyStore<E> doesn't need to know all the concrete types of all of its properties.

What I expected is that the function stored in value_store_constructor would already construct the property value storage in a way that respects the Property::default_index_type() value—or more precisely, I would expect PropertyValueStoreCore::<E, P>::new_boxed itself to respect the Property::default_index_type() value. The property registry doesn't need to be involved at all, as far as I can tell.

Am I missing something?

[github-actions]

Benchmark Results

Hyperfine

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
large_sir::baseline	3.4 ± 0.1	3.4	3.6	1.00
large_sir::baseline_households	3.4 ± 0.0	3.3	3.6	1.00 ± 0.02
large_sir::entities	8.0 ± 0.1	7.8	8.3	2.33 ± 0.05
large_sir::households	9.0 ± 1.8	7.4	15.5	2.61 ± 0.54

Criterion

Regressions (slower)

Group	Bench	Param	Change	CI Lower	CI Upper
counts	reindex_after_adding_more_entities		109.261%	108.630%	109.911%
large_dataset	bench_filter_indexed_entity		73.853%	70.210%	77.694%
counts	index_after_adding_entities		31.870%	31.476%	32.249%
sampling	count_and_sampling_single_unindexed_concrete_plus_derived_entiti		20.762%	20.504%	21.020%
sampling	sampling_single_unindexed_concrete_plus_derived_entities		19.387%	18.515%	20.199%
sample_entity	sample_entity_single_property_unindexed	100000	18.297%	16.713%	19.930%
sample_entity	sample_entity_multi_property_indexed	10000	18.224%	17.317%	19.186%
sample_entity	sample_entity_multi_property_indexed	100000	18.044%	16.252%	19.961%
sample_entity	sample_entity_multi_property_indexed	1000	16.624%	14.559%	18.504%
indexing	query_people_indexed_multi-property_entities		15.776%	15.342%	16.164%
counts	single_property_indexed_entities		7.673%	7.141%	8.095%
sample_entity	sample_entity_single_property_unindexed	1000	4.754%	3.488%	6.102%
sample_entity	sample_entity_single_property_unindexed	10000	3.572%	3.316%	3.928%
counts	concrete_plus_derived_unindexed_entities		3.137%	1.652%	4.832%
examples	example-births-deaths		1.734%	1.459%	1.996%

Improvements (faster)

Group	Bench	Param	Change	CI Lower	CI Upper
large_dataset	bench_query_population_multi_unindexed_entities		-99.395%	-99.402%	-99.390%
indexing	with_query_results_indexed_multi-property_entities		-36.649%	-36.798%	-36.416%
algorithm_benches	algorithm_sampling_single_known_length		-27.162%	-33.151%	-20.401%
indexing	with_query_results_single_indexed_property_entities		-21.531%	-24.693%	-18.263%
sampling	count_and_sampling_single_known_length_entities		-19.144%	-21.824%	-16.327%
counts	multi_property_indexed_entities		-13.411%	-13.856%	-12.958%
large_dataset	bench_query_population_indexed_property_entities		-9.077%	-10.173%	-8.216%
sampling	sampling_multiple_known_length_entities		-6.798%	-9.533%	-4.261%
sampling	sampling_single_unindexed_entities		-6.196%	-6.350%	-5.970%
large_dataset	bench_query_population_multi_indexed_entities		-4.958%	-5.363%	-4.571%
sample_entity	sample_entity_whole_population	100000	-4.763%	-7.607%	-1.813%
large_dataset	bench_query_population_property_entities		-3.471%	-5.248%	-1.968%
indexing	with_query_results_multiple_individually_indexed_properties_enti		-2.940%	-3.501%	-2.408%
indexing	query_people_multiple_individually_indexed_properties_entities		-2.913%	-3.247%	-2.581%

Unchanged / inconclusive (CI crosses 0%)

Group	Bench	Param	Change	CI Lower	CI Upper
large_dataset	bench_filter_unindexed_entity		-4.546%	-8.731%	-0.278%
sample_entity	sample_entity_single_property_indexed	100000	-3.344%	-6.258%	-0.200%
sample_entity	sample_entity_whole_population	10000	-3.262%	-6.320%	-0.067%
sample_entity	sample_entity_whole_population	1000	2.451%	-1.144%	6.553%
sample_entity	sample_entity_single_property_indexed	1000	-2.358%	-6.611%	2.258%
sampling	sampling_single_known_length_entities		-1.538%	-2.536%	-0.583%
counts	single_property_unindexed_entities		1.312%	0.256%	2.546%
indexing	query_people_count_single_indexed_property_entities		-1.268%	-2.065%	-0.643%
large_dataset	bench_query_population_derived_property_entities		1.267%	-0.505%	2.866%
examples	example-basic-infection		-1.005%	-1.520%	-0.414%
large_dataset	bench_match_entity		-1.003%	-1.396%	-0.592%
sampling	sampling_multiple_l_reservoir_entities		-0.992%	-1.588%	-0.558%
sample_entity	sample_entity_single_property_indexed	10000	0.879%	-2.211%	4.075%
indexing	query_people_count_indexed_multi-property_entities		-0.701%	-1.251%	-0.327%
indexing	query_people_single_indexed_property_entities		-0.487%	-3.715%	2.345%
algorithm_benches	algorithm_sampling_single_rand_reservoir		-0.410%	-1.032%	0.018%
indexing	query_people_count_multiple_individually_indexed_properties_enti		0.316%	0.035%	0.594%
counts	multi_property_unindexed_entities		0.168%	-0.292%	0.755%
sampling	sampling_multiple_unindexed_entities		-0.123%	-0.169%	-0.074%
algorithm_benches	algorithm_sampling_multiple_known_length		0.112%	-0.702%	0.854%
sampling	sampling_single_l_reservoir_entities		0.087%	-0.268%	0.342%
algorithm_benches	algorithm_sampling_multiple_l_reservoir		-0.057%	-0.609%	0.503%
algorithm_benches	algorithm_sampling_single_l_reservoir		0.012%	-0.297%	0.436%

Not Compared (no baseline yet)

Group	Bench	Reason
(none)

[cdc-as81]

This PR is a lot more invasive than I expected, and I can't tell if it is because I don't fully understand some edge cases or if the complexity is genuinely unnecessary.

The PR adds:

• The single method add_to_property_registry is now expanded to add_indexed_to_property_registry and add_to_property_registry_with_constructor.
• PropertyValueStoreCore gets a new constructor new_boxed_with_default_index.
• PropertyMetadata gets a new field index_new_entity_fn

The index_new_entity_fn field is the most suspicious to me. When a new property is registered (before main executes), it stores a constructor in value_store_constructor that can construct a new property value store for the property in a type erased way. This constructor is used in the constructor of a PropertyStore<E> (the thing that holds every property value store for every property of an entity) so that the PropertyStore<E> doesn't need to know all the concrete types of all of its properties.

What I expected is that the function stored in value_store_constructor would already construct the property value storage in a way that respects the Property::default_index_type() value—or more precisely, I would expect PropertyValueStoreCore::<E, P>::new_boxed itself to respect the Property::default_index_type() value. The property registry doesn't need to be involved at all, as far as I can tell.

Am I missing something?

Thanks—there is one constraint here that was not obvious from the previous
implementation.

I agree that default-index construction belongs with property-value-store
construction. I refactored the code accordingly:

• PropertyMetadata no longer has a separate index_new_entity_fn field. It
  now stores only the
  constructor.
• The constructor returns the property value store and its optional new-entity
  dispatcher as one value, so they cannot get out of sync
  (source).
• new_boxed_with_default_index has been removed. The indexable constructor
  evaluates P::default_index_type() once and passes the selected index into
  new_boxed_with_index
  (source,
  store constructor).

The reason PropertyValueStoreCore::<E, P>::new_boxed() cannot directly honor
P::default_index_type() is the trait bound introduced by constructing an
index.

new_boxed() must work for every property:

impl<E: Entity, P: Property<E>> PropertyValueStoreCore<E, P> {
    fn new_boxed() -> Box<dyn PropertyValueStore<E>> {
        // ...
    }
}

However, constructing either index requires:

P: IndexableProperty<E> // Property<E> + Eq + Hash

That requirement is visible in
PropertyIndexType::new_property_index
and the
IndexableProperty definition.

Rust cannot acquire the Eq + Hash bound conditionally after inspecting the
runtime PropertyIndexType. Putting this directly in the general constructor
would therefore require either:

1. Restoring Eq + Hash as a requirement for every Property, undoing the
   change from #937, or
2. Adding another construction hook to the public Property trait and every
   property macro.

The two registration entry points are the narrow compile-time bridge between
those bounds:

• Ordinary P: Property<E> values register an unindexed constructor.
• Multi-properties, which satisfy P: IndexableProperty<E>, register the
  constructor that honors default_index_type().

The relevant
registration split
is implemented in property_store.rs.
The registry still contains only type-erased construction behavior; it does not
contain or share an index instance.

The dispatcher is required because constructing an empty index is not
sufficient to keep it current. After a new entity's explicit values have been
assigned, the dispatcher computes the derived or multi-property value and
inserts the entity into the index
(entity creation,
dispatcher).

There is regression coverage showing that:

• Every constructed indexed store has its matching dispatcher
  (test).
• Different Context instances receive independent indexes
  (test).
• Explicitly upgrading an unindexed property indexes existing entities and
  continues indexing subsequently added entities
  (test).

So your expectation is correct in principle: the registered constructor should
produce a store that respects the configured default. The remaining
specialized registration path exists only to retain the narrower Property
bound established by #937 while constructing indexes for types known to
implement IndexableProperty.