fix: skip reservation injection when preOps has an outer DECLARE#65
Open
steps-re wants to merge 1 commit into
Open
fix: skip reservation injection when preOps has an outer DECLARE#65steps-re wants to merge 1 commit into
steps-re wants to merge 1 commit into
Conversation
When a table/view/incremental sets its own preOps via a chained
`.preOps('DECLARE ...')` after `publish()`, the reservation
`SET @@reservation=...` statement was injected at build time, before the
DECLARE existed. The result was `[SET, DECLARE]` in the compiled preOps,
which BigQuery rejects because a DECLARE must be the first statement in a
script.
Monkeypatch the builder's `.preOps()` (mirroring the existing `.queries()`
patch for operations) so that when a later-supplied outer DECLARE is
detected, any reservation statement we already injected is removed. The
statement can land in `proto.preOps` (Dataform v3) or `contextablePreOps`
(Dataform v2), so both containers are cleaned.
This fixes the `test_incremental` DECLARE-skip integration check that was
failing the CI matrix for both Dataform 2.4.2 and 3.0.48.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Mike German <mike@stepsventures.com>
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
1 participant
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
Reservation injection produces invalid BigQuery SQL for any model whose
preOpsstarts with aDECLARE, and this is currently breaking the CI test matrix on every PR.Details
The package injects
SET @@reservation='...';into an action's pre-operations atpublish()time. When a model supplies its own preOps via a chained call —publish(...).preOps('DECLARE ...').query(...)— the reservation statement lands before the user'sDECLARE, compiling to["SET @@reservation=...", "DECLARE ..."]. BigQuery rejects this because aDECLAREmust be the first statement in a script, so the model fails to run.This is exactly the failure making the CI matrix (Dataform
2.4.2/3.0.48) red on every PR — e.g.Table test_incremental should NOT have a reservation injected, but found one in preOps. CI only runs onpull_request, somainnever exercises the matrix and it went unnoticed.Fix
Monkeypatch the builder's
.preOps()(mirroring the existing.queries()patch) so a later-supplied outerDECLAREremoves the reservation statement we injected. Handles both Dataform v3 (proto.preOps) and v2 (contextablePreOps).Verification
main:./scripts/test-matrix.shfails at 2.4.2 with thetest_incrementalDECLARE error; confirmed identical string in the repo's own CI logs.npx jest→ 89/89 pass;./scripts/test-matrix.sh→ SUCCESS for both 2.4.2 and 3.0.48. Normal reservation tables/views/operations still get their reservation injected; only DECLARE-carrying models are skipped.eslintexit 0.This turns the PR CI matrix green (and unblocks the pending dependency PRs). Worth noting CI runs only on
pull_request, which is whymainhid the regression.