fix: run filter-feature intake before the filter enters the collection - #909
Merged
Merged
Conversation
A matched filter feature was stored in GlobalFilter.collection and only then run through feature intake. When the filter feature declares a defaulted PROPERTY_MAPPING key explicitly as None, unify_options leaves that None in place and options_with_defaults fills the declared default, so intake rebound the options of a Feature already inside a set. SingleFilter's hash covers that feature, so the stored entry became unfindable in its own set and a host feature reached by two consumers stored the same filter twice, handing both to the FeatureSet. Swap the two calls so the options are final before the filter is stored, and replace the comment that asserted the intake call was an identity no-op.
…ring Two hosts of one feature group, one reached twice and one once, made the pre-fix ordering abort the run: the collection entries had unequal lengths and ExecutionPlan.add_single_filters_to_feature_set rejected them. The symmetric cases could not reach that, because set equality compares stored hashes and two equally stale sets still compare equal, while set membership recomputes. Also state which tests are guards and which fail pre-fix, and reword the engine comment: a group fill shifts SingleFilter's hash, a context fill only its equality, since Options hashes group alone.
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
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.
Closes #904.
The defect
Engine._add_filter_featurestored a matchedSingleFilterintoGlobalFilter.collectionand only then ran the filter feature through feature intake. The comment above that call claimed the intake call "must remain an identity no-op (no rebind) to keep the SingleFilter set hash stable". It is not.GlobalFilter.unify_optionscopies the host's effective options into the filter feature only for keys the filter feature does not already hold, so a key the filter feature declares explicitly asNonereaches intake unfilled.options_with_defaultsthen treats a present-but-Nonevalue as absent and fills the declared default, rebinding the options of aFeaturethat is already inside a set.GlobalFilter.add_filteraccepts aFeature, so this is reachable through the public API.Two failure modes, because
Options.__hash__covers group only whileFeature.__eq__compares group and context:context=False)SingleFilter.__hash__shifts, the set stops finding its own membercontext=True)Feature.__eq__breaksObservable symptoms, both reproduced against the pre-fix ordering:
ExecutionPlan.add_single_filters_to_feature_sethands both to the FeatureSet.ValueError: ... has different filters for different features.The symmetric two-host case stays silent because CPython's
set.__eq__compares using the stored entry hashes, so two equally stale sets still compare equal, whileset.__contains__recomputes.The fix
Swap the two calls so the filter feature's options are final before the
SingleFilteris stored, and replace the stale comment. Every other mutation of the filter feature's hash-relevant fields (optionsviaunify_options,domain,compute_frameworks,name) already happened before the insert; this closes the last one.The effective options are unchanged: the filter feature still computes with the materialized default, which a guard test pins.
Tests
New module
tests/test_core/test_filter/test_filter_feature_explicit_none_intake.py, 8 tests. Four discriminate (they fail against the pre-fix ordering), four are guards that pin behavior a wrong fix would break, notably that anallow_explicit_none=TrueexplicitNoneis still honored and never filled. Each docstring says which it is.Full
toxgate green: 7543 passed, 170 skipped, ruff, mypy --strict, licenses, bandit.