Skip to content

fix: skip reservation injection when preOps has an outer DECLARE#65

Open
steps-re wants to merge 2 commits into
masthead-data:mainfrom
steps-re:fix/declare-preops-reservation-skip
Open

fix: skip reservation injection when preOps has an outer DECLARE#65
steps-re wants to merge 2 commits into
masthead-data:mainfrom
steps-re:fix/declare-preops-reservation-skip

Conversation

@steps-re

@steps-re steps-re commented Jul 7, 2026

Copy link
Copy Markdown

Summary

Reservation injection produces invalid BigQuery SQL for any model whose preOps starts with a DECLARE, and this is currently breaking the CI test matrix on every PR.

Details

The package injects SET @@reservation='...'; into an action's pre-operations at publish() time. When a model supplies its own preOps via a chained call — publish(...).preOps('DECLARE ...').query(...) — the reservation statement lands before the user's DECLARE, compiling to ["SET @@reservation=...", "DECLARE ..."]. BigQuery rejects this because a DECLARE must 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 on pull_request, so main never exercises the matrix and it went unnoticed.

Fix

Monkeypatch the builder's .preOps() (mirroring the existing .queries() patch) so a later-supplied outer DECLARE removes the reservation statement we injected. Handles both Dataform v3 (proto.preOps) and v2 (contextablePreOps).

Verification

  • Reproduced on main: ./scripts/test-matrix.sh fails at 2.4.2 with the test_incremental DECLARE error; confirmed identical string in the repo's own CI logs.
  • After fix: npx jest89/89 pass; ./scripts/test-matrix.shSUCCESS 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. eslint exit 0.

This turns the PR CI matrix green (and unblocks the pending dependency PRs). Worth noting CI runs only on pull_request, which is why main hid the regression.

@max-ostapenko

Copy link
Copy Markdown
Contributor

@steps-re Thanks for raising this!

Though it would be a safer approach to avoid injecting the reservation in the first place, keeping it completely clean without having to "delete" or strip anything from the SQL.

It would be a direct equivalent to how action.queries handles operations. Since every valid table/view builder has both a .preOps() and a .query() method, we can intercept both at creation time.

Could you adjust this?

@max-ostapenko
max-ostapenko self-requested a review July 11, 2026 21:31

@max-ostapenko max-ostapenko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid injecting the reservation in the first place.

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 compiled preOps became [SET, DECLARE], which BigQuery
rejects because a DECLARE must be the first statement in a script.

Rather than injecting eagerly and stripping the statement back out when a
DECLARE appears, mirror the operations .queries() patch: intercept the
builder's .preOps() and .query() and inject the reservation only when there
is no outer DECLARE. Every valid table/view builder exposes both methods, so
the reservation is prepended to user preOps that have no DECLARE, added on
.query() when there are no preOps at all, and never injected when the user
supplies a leading DECLARE. Nothing is deleted after the fact.

Compiled-object and already-resolved-builder paths are unchanged; they fall
through to the existing direct-injection logic. jest 89/89 pass and the
Dataform 2.4.2 / 3.0.48 integration matrix (including the DECLARE-skip
check) is green.

Signed-off-by: Mike German <mike@stepsventures.com>
@steps-re
steps-re force-pushed the fix/declare-preops-reservation-skip branch from fc52452 to b36a8a1 Compare July 12, 2026 00:26
@steps-re

Copy link
Copy Markdown
Author

Thanks @max-ostapenko, that's cleaner. Reworked to avoid injecting the reservation in the first place instead of stripping it back out.

It now mirrors the operations .queries() patch: I intercept the builder's .preOps() and .query() and inject the reservation only when there's no outer DECLARE. Concretely:

  • .preOps() with no DECLARE → prepend the reservation to the user's preOps.
  • .preOps('DECLARE ...') → passed through untouched, reservation never added.
  • .query() with no preOps at all → reservation injected then (no DECLARE possible).

Nothing gets deleted after the fact. The compiled-object / already-resolved paths are unchanged and still use direct injection. jest is 89/89 and the 2.4.2 / 3.0.48 matrix (including the DECLARE-skip check) is green. Ready for another look.

Cover the interception-at-creation design for table/view/incremental
builders so the reservation is injected only where it is safe, mirroring
the operations .queries() patch. The builder's .preOps() and .query() are
intercepted at creation time:

- no preOps at all      -> reservation injected via .query() (no DECLARE
                           possible on that path)
- preOps with a DECLARE -> passed through untouched, reservation never added
- normal preOps         -> reservation prepended before the user's preOps

This replaces the previous inject-then-strip behaviour: nothing is ever
emitted before a DECLARE and nothing is deleted from the SQL afterward.

The prior commit added the interception logic but left the new .preOps()/
.query() path with zero test coverage (jest mocks exposed no builder with
both methods, so the block never ran). These tests exercise that path with
a realistic builder that writes through to proto.preOps/proto.query, and a
mutation check confirms the DECLARE-skip assertion genuinely gates it. jest
now 93/93 green and index.js coverage rises 89.75% -> 98.79%; the Dataform
2.4.2 / 3.0.48 integration matrix (including the test_incremental DECLARE
case) also passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants