fix(data_operations): align PROPERTY_MAPPING declarations with runtime dispatch - #330
Open
TKaltofen wants to merge 2 commits into
Open
fix(data_operations): align PROPERTY_MAPPING declarations with runtime dispatch#330TKaltofen wants to merge 2 commits into
TKaltofen wants to merge 2 commits into
Conversation
…e dispatch Frame aggregation now composes the PROPERTY_MAPPING matcher instead of replacing it: the four name patterns become the class prefix patterns, the name path validates through _validate_string_match, and the override keeps only the per-subclass capability and partition/order shape checks. frame_size and frame_unit gain declared value spaces and required_when rules, so a config-based window is rejected at match time rather than at compute. Rank and offset declare their parametric families through one predicate shared by the name path and the config path, so ntile_N, top_N, bottom_N, lag_N, lead_N, diff_N and pct_change_N match identically from a feature name or from Options. offset_type becomes strict, which ends the acceptance of arbitrary strings such as "banana". MatchValidationTestBase gains opt-in parity_operations and malformed_operations hooks that assert both creation paths agree, and is_positive_int moves to the shared data_operations base.
…eview
Zero-sized frames matched through the name path while the config path
rejected frame_size=0, so the rolling and time-window patterns now carry
[1-9]\d* like binning does. A frame name encodes its own size and unit, so
the new required_when rules are exempted on that path and a name-based
feature that also carries frame_type in its Options keeps matching.
A single-element container is legal syntax for one operation token, but the
dispatch-side readers stringified it into "('sum',)"; op_token_value unwraps
it for frame, rank and offset, including the capability-hook resolvers.
A parametric suffix is now an ASCII decimal: str.isdigit accepts superscripts
that int() then rejects, which raised out of the matcher, and it accepts
non-ASCII digits that reached compute. Hostile in_features values raised from
core's unguarded count check instead of failing to match, so all three
families guard that entry.
This was referenced Jul 28, 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
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 #326.
What
Makes the
PROPERTY_MAPPINGdeclaration the authority for frame aggregation, rank and offset, so a supported operation is accepted the same way whether it arrives in a feature name or inOptions.Frame aggregate composed the base matcher instead of replacing it. Its four name shapes (rolling, time window, cumulative, expanding) became the class prefix patterns, the name path validates through
_validate_string_match, and the override keeps only what a shared class-level declaration cannot express: the per-subclassSUPPORTED_FRAME_TYPES/SUPPORTED_TIME_UNITSnarrowing and thepartition_by/order_byshape checks.aggregation_typeandframe_typedroppeddefault: Noneso the config path really requires them,frame_sizegained a positive-integer guard plusrequired_when, andframe_unitgained its declared value space plusrequired_when. A frame name carries its own size and unit, so those conditional requirements are exempted on the name path.Rank and offset declare their parametric families through one predicate shared by both paths, so
ntile_N,top_N,bottom_N,lag_N,lead_N,diff_Nandpct_change_Nnow match identically from a name or fromOptions.offset_typebecame strict, which ends the acceptance of arbitrary strings such as"banana".Zero and negative suffixes, malformed parametric tokens and unsupported operations are now rejected while matching rather than at compute.
Review findings folded in
Two independent deep reviews ran on the first commit; the second commit is their triage:
sales__sum_rolling_0,sales__avg_0_day_window) matched on the name path and either crashed or silently computed a degenerate window; the size group is now[1-9]\d*, as binning already does"('sum',)";op_token_valueunwraps it for all three families and for the capability-hook resolversstr.isdigit()accepts superscripts thatint()then rejects, sovalue__ntile_²_rankedraised out of the matcher and aborted resolution; it also accepts non-ASCII digits that reached compute. A parametric suffix is now an ASCII decimalin_featuresvalues ("",0,3.5,True, a dict) raised from core's unguarded count check instead of failing to match; all three families guard that entryTests
MatchValidationTestBasegained opt-inparity_operations()/malformed_operations()hooks that assert both creation paths agree, wired for the three families in scope. Frame adds a local parity test over all four of its name shapes, since the shared harness builds only the rolling one. Fulltoxgate green: 4442 passed, 168 skipped, plus ruff format, ruff check, strict mypy and bandit.