fix(deploy): preserve is_init() branching in --target rewrites - #9876
Conversation
`rewrite_for_isolated` and `rewrite_for_defer` render query.sql to find
table refs, then write the rewritten result back. `is_init` defaults to
False, so the render collapsed `{% if is_init() %}` down to its `else`
arm and the init query was lost from the deployed copy for good.
CI's stage deploy runs `deploy --isolated`, and Test SQL executes the
staged copy: the SQL pytest plugin re-renders it with `is_init=True` for
a `test_init` directory. With the branching gone it silently got the
incremental query instead, filtered to a single `@submission_date`, so
any test_init expectation covering more than one submission_date failed.
`query initialize` on a target was affected the same way.
Render both arms, rewrite each, and re-wrap them in the conditional —
what the legacy `stage deploy` path did before it was replaced.
Surfaced by org_mozilla_fenix_derived.baseline_clients_daily_v1/test_init
on PR #9855, which stages the table via the retention_clients views. 8 of
the 9 tables with a test_init use is_init() branching, so all were
latently affected.
91d1747 to
ed74d2b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This extracts the ref-rewriting bodies of rewrite_for_isolated and rewrite_for_defer into closures and routes both through a new _render_and_rewrite helper, which renders query.sql once per is_init() arm, rewrites each, and re-wraps them in {% if is_init() %} / {% else %} so the deployed copy keeps its branching. Four regression tests cover the isolated path.
The approach reads correctly to me: re-wrapping is idempotent (on a second pass the target-project refs are skipped by the parts[0] == target_project guard), and the non-branching fast path is unchanged. My comments are about the cost of calling the rewrite callback twice in the defer path, and about how much the is_init() substring guard is relied on given that a miss fails silently.
Checklist items: no schema or backfill.yaml changes, no new derived datasets, and this is a branch on the main repo rather than a fork, so the fork-review item doesn't apply. Worth adding a bug/issue reference to the PR title or description per the first checklist item — the description points at PR #9855 but no ticket.
| # branching pulled in via an {% include %} still take the two-arm path; the | ||
| # equality check below keeps the fast, single-copy result when the mention | ||
| # doesn't actually change the output (e.g. it's only in a comment). | ||
| if "is_init" not in query_file.read_text(): |
There was a problem hiding this comment.
How expensive is a render? I'm wondering if something like if _render(is_init=lambda: True) != _render(is_init=lambda: False) is feasible and might be more reliable. I'm guessing that this most affects stage deploys
There was a problem hiding this comment.
Rendering is pretty cheap (a few ms). sqlglot (rewrite) is taking a little longer depending on the file (up to 10s to 100s of ms).
I made the change to compare the outputs vs searching for is_init since that does seem more reliable
2 participants
rewrite_for_isolatedandrewrite_for_deferrender query.sql to find table refs, then write the rewritten result back.is_initdefaults to False, so the render collapsed{% if is_init() %}down to itselsearm and the init query was lost from the deployed copy for good.CI's stage deploy runs
deploy --isolated, and Test SQL executes the staged copy: the SQL pytest plugin re-renders it withis_init=Truefor atest_initdirectory. With the branching gone it silently got the incremental query instead, filtered to a single@submission_date, so any test_init expectation covering more than one submission_date failed.query initializeon a target was affected the same way.Render both arms, rewrite each, and re-wrap them in the conditional — what the legacy
stage deploypath did before it was replaced.Surfaced by org_mozilla_fenix_derived.baseline_clients_daily_v1/test_init on PR #9855, which stages the table via the retention_clients views. 8 of the 9 tables with a test_init use is_init() branching, so all were latently affected.
Test branch: #9879
Reviewer, please follow this checklist