docs: state that match_feature_group_criteria's options view depends on the caller - #903
Merged
Merged
Conversation
…on the caller The classmethod is called on two paths with two views of Options. Feature resolution passes declared (pre-default) options, because it runs before intake. GlobalFilter calls the same method again to match a filter feature, after intake has materialized declared defaults and unify_options has carried them onto the filter feature, so that call observes effective (post-default) options. Nothing in the method's own docstring said so, which is the surface an author overriding it actually reads. An override whose matching logic inspects option values can behave differently between the two paths with no warning. The property-mapping page also asserted that materialization never changes matching, which its own filter-matching paragraph contradicts. Qualify it to resolution matching, and cross-reference the rule from the matching guide.
…alized view Review correction. GlobalFilter.criteria passes the filter feature's options after unify_options, which copies the resolved feature's effective options only for keys the filter feature does not already set. It never materializes the filter feature's own options. So a key the filter feature declares itself keeps its declared value, and only an omitted key arrives materialized. Calling that view "effective (post-default) options" overstates it, and add_filter accepts a Feature, so a caller can reach the difference. Also carry the rule to the surfaces that were still missing it: FeatureChainParserMixin overrides this classmethod with its own docstring, so every group on the recommended authoring path showed nothing; the stage table said a bare "match" observes declared options, contradicting its own filter-matching row; and the matching guide gated the rule behind overriding, though a group using the inherited matcher is affected too.
This was referenced Jul 30, 2026
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
2 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
FeatureGroup.match_feature_group_criteriahas two production call sites, and they hand it two different views ofOptions:IdentifyFeatureGroupClass._filter_feature_group_by_criteriaresolves a feature, which runs before intake, so it passes declared (pre-default) options.GlobalFilter.criteriamatches a filter feature, which runs after intake, so the values it passes have already been throughoptions_with_defaultson the resolved feature.Nothing in the classmethod's own docstring said so, which is the surface an author overriding it actually reads. Matching logic that inspects option values can behave differently between the two paths with no warning that this is even possible.
What changed
Docs and docstrings only. No behavior change, no new tests: the contract is already pinned by
tests/test_core/test_filter/test_filter_criteria_effective_options.py.FeatureGroup.match_feature_group_criteriagains a paragraph describing the rule.FeatureChainParserMixin.match_feature_group_criteriagains a two-line pointer to it. The mixin overrides the classmethod with its own docstring, so every group on the recommended authoring path was showing nothing.in_depth/feature-group-matching.mdstates the rule and links to the stage table.in_depth/property-mapping.md: the page asserted that materialization "never changes matching" while its own filter-matching paragraph said the opposite, and the stage table listed a bare "match" as observing declared options. Both are disambiguated to resolution matching.Precision worth noting
The second commit corrects an overstatement in the first. The filter path does not receive a materialized view of the filter feature's options.
GlobalFilter.unify_optionscopies the resolved feature's effective options only for keys the filter feature does not already set, and never callsoptions_with_defaultson the filter feature itself. Verified against a spec declaringdefault="D":{'k': 'D'}(materialized, from the resolved feature){'k': None}{'k': None}(its own declared value survives){'k': 'OWN'}{'k': 'OWN'}GlobalFilter.add_filteraccepts aFeature, so a caller can reach that difference. The docs now describe it as a merge rather than a materialized view.Review
Two independent deep reviews.
toxgreen: 7535 passed, 170 skipped, ruff, mypy --strict, bandit clean.Two findings were real but outside a docs commit and will be filed separately:
in_depth/filter_data.mddescribes filter matching inaccurately, and the comment atmloda/core/core/engine.py:337-339asserts the filter-feature intake call "must remain an identity no-op", which does not hold when a filter feature declares a defaulted key explicitly asNone.