Repository navigation
Scope legacy variable selection by databaseStart (0.5.0 release review) - #65
Merged
Merged
Conversation
Found in a second release review of 0.5.0. The legacy path chose which variables to generate from detail rows alone and ignored variable-level databaseStart, which the v0.4 adapter honours. So a formula or survival variable listed only for another database made the new guards stop generation for the requested one, and a plain variable listed only for another database was generated as random values, in every detail layout tested (no detail databaseStart, shared rows, other-only rows). The legacy path now filters enabled variables by variable-level databaseStart with the adapter's matcher and empty-means-all rule, before the guards run. Scoping only the guards' candidates would have removed the false stop but left the legacy path generating another database's formula variable as random values. This also corrects an earlier ruling that the survival guard's lack of databaseStart scoping could stand.
Merged
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.
Summary
A second external review of the release candidate (
f8d012a) found a regression in #64's formula guard, and it was reproduced here. A formula variable listed only for another database stopped generation for the requested database on the legacy path. The survival guard from #61 had the same flaw.Root cause, wider than the guards: the legacy path chose variables from detail rows alone and ignored variable-level
databaseStart, which the v0.4 adapter honours. A variable listed only forotherwas generated forstudyas random values, in every detail layout tested: nodatabaseStartin the details, shared detail rows, andother-only detail rows.Fix: the legacy path filters enabled variables by variable-level
databaseStartwith the adapter's matcher (.filter_recodeflow_by_database(), empty means all databases) before the guards run.The review suggested scoping only the guards' candidates. That would remove the false stop, but it would leave the legacy path generating the other database's formula variable as random values, the silent-wrong output the guard exists to prevent. So this fixes the selection instead.
This also reverses a ruling I made during #61's final review ("the D10 guard checks role-filtered rows, not databaseStart-filtered; stands"). The reproduction shows it could not stand.
Tests
the legacy path generates only variables that belong to the requested databasecovers:otherother-only detail rowsIt was RED before the fix: the plain variable was generated, and the formula guard stopped the run.
Verification (local, R 4.4.2)
One pre-existing limitation seen while testing, not changed here: when
variableshas adatabaseStartcolumn andvariable_detailsdoes not, the v0.4 adapter stops with "databaseStart filtering was requested, but metadata has no 'databaseStart' column".After this merges into
dev, release PR #63 picks it up; rerun its checks before merging and tagging.🤖 Generated with Claude Code