Tracking: SDK production cleanup — rename migration_v5, consolidate context-graph surface, blogpost-ready audit

[caohy1988]

Tracking: SDK production cleanup — rename migration_v5, consolidate context-graph surface, blogpost-ready audit

Context

We are taking the SDK to production-ready in preparation for a public blogpost that highlights the context graph capabilities. The current repo layout has a few rough edges that block that:

• examples/migration_v5/ is a load-bearing demo (MAKO ontology + the ontology-agnostic artifact pipeline + a runnable agent + the scheduled Cloud Run deployment), but its name does not describe what it does. "v5" was an internal milestone label; for an external reader landing from a blogpost it reads as version bookkeeping for something they have no context on.
• examples/migration_v5/periodic_materialization/ will be promoted to adk.dev/integrations/bigquery-agent-analytics/ as user-manual content (see companion issue). The deep-link path that users land on cannot be migration_v5/....
• The context-graph design docs are versioned in their filenames (docs/context_graph_v2_design.md, docs/context_graph_v3_design.md) rather than canonical + archive, so readers cannot tell which one to read.
• The context-graph source surface is wider than it needs to be (20+ files across src/bigquery_agent_analytics/), with a mix of public API, internal helpers (one _-prefixed), and a _v2/_v3 design history that has bled into module names in a few places.
• migration_v5 is referenced in 36 paths across the repo (file names, directory names, generated artifact paths, test names, deploy script literals), so the rename is non-trivial and needs to be done as one coordinated change.

This issue is the umbrella for that cleanup. Sub-issues will be filed for each renamable / auditable surface.

Audit findings

1. The migration_v5 name

From examples/migration_v5/README.md:

The demo's event source of truth is a runnable agent talking to the BQ AA plugin ... The artifact pipeline that turns a TTL into binding + DDL + property-graph SQL is ontology-agnostic ... and the MAKO config (in mako_artifacts.py) is one concrete configuration of it.

The demo is the ontology-driven context-graph extraction pipeline, with MAKO as the load-bearing example. Concrete rename candidates:

Candidate	Pros	Cons
examples/context_graph_demo/	Matches the blogpost framing; obvious from a deep link	Mild overlap with src/bigquery_agent_analytics/context_graph.py (module) — namespace, not collision
examples/ontology_driven_context_graph/	More accurate description	Long for deep links
examples/mako_context_graph/	Names the load-bearing example	Bakes one config into the demo's directory name, but the pipeline is supposed to be ontology-agnostic
examples/context_graph_extraction/	Verb-based; matches "extraction" language	Slightly less obvious than _demo for someone exploring examples/

Recommended: examples/context_graph_demo/. It matches the blogpost framing, scales to multiple ontology configs (which is the pluggable-contract direction the README already documents), and produces clean deep links like examples/context_graph_demo/periodic_materialization/README.md.

2. Files / paths to rename

36 paths today contain migration_v5. The rename touches:

• examples/migration_v5/ → examples/context_graph_demo/ (whole directory tree)
• examples/migration_v5_demo_notebook.ipynb → examples/context_graph_demo_notebook.ipynb
• tests/test_migration_v5_ontology_artifacts.py and any other test_migration_v5_*.py → test_context_graph_demo_*.py
• Generated dataset names like migration_v5_demo that appear in binding.yaml, table_ddl.sql, property_graph.sql — these are user-visible BigQuery dataset names. Decide: rename the generated default to context_graph_demo, or keep the legacy default with a deprecation note for users with existing tables.
• examples/migration_v5/periodic_materialization/README.md references — every internal cross-link inside the deploy scripts, Terraform, and the README needs updating.
• Test fixtures that include the path in golden output / snapshots.

3. Design-doc consolidation

docs/context_graph_v{2,3}_design.md (50KB + 28KB) and docs/learning_ontology_and_context_graph.md (25KB). Three problems:

• Versioned filenames imply the reader needs to know which is current.
• The "learning" doc reads like an explainer; the v3 design doc reads like a technical reference. They have overlapping scope but different audiences.
• The blogpost will link readers to one canonical doc; today there isn't one.

Recommended consolidation:

• docs/context_graph.md (canonical user-facing concept overview, sourced from the learning doc + the user-facing parts of v3).
• docs/context_graph_design.md (technical reference, sourced from v3's design-doc material).
• docs/_archive/context_graph_design_v2.md (preserved for history; out of the documented surface).

4. Context-graph source surface audit

src/bigquery_agent_analytics/ carries these context-graph-related modules:

context_graph.py
_ontology_routing.py
extracted_models.py
extractor_compilation/ (15 files)
graph_validation.py
materialize_window.py
ontology_graph.py
ontology_materializer.py
ontology_models.py
ontology_orchestrator.py
ontology_property_graph.py

Open questions:

• _ontology_routing.py has the leading-underscore internal convention but lives in the public package. Decide: move to _internal/ (most common Python convention) or drop the underscore and treat it as public.
• ontology_graph.py vs. ontology_property_graph.py vs. context_graph.py — three modules with overlapping mental-model names. Is each one a distinct concept? Audit and (if duplication exists) merge or document the distinction.
• ontology_orchestrator.py and ontology_materializer.py — separate concerns or one renamed in flight?
• extractor_compilation/ — 15 files. Verify which are public API and which should be in a sub-package internal sentinel (_compiler/, etc.).
• materialize_window.py is the documented public API the periodic-materialization deploy depends on (per periodic_materialization/README.md). Make sure its docstring states that explicitly so the rename audit doesn't accidentally hide it.

The audit should produce a written docs/context_graph_surface_audit.md listing every module with: public vs. internal, primary concept, related modules, and a one-line summary. That doc becomes the input for any renaming/merging follow-ups.

5. README + SDK.md alignment

• README.md is 7KB. For a public blogpost landing page it needs a clear "what is this, and what is the context graph" lede plus a 30-second quickstart pointing at examples/context_graph_demo/.
• SDK.md is 64KB. Too long to be the front door. Consider:
  • Splitting into docs/sdk_reference.md (API reference) + docs/concepts.md (concepts) + retaining SDK.md as a thin landing-page index.
  • Or keeping it as one doc but adding a clear table-of-contents and section anchors so a blogpost can deep-link.

6. Notebook hygiene

• examples/context_graph_adcp_demo.ipynb — "adcp" is an unexpanded acronym for someone landing fresh. Either expand it in the filename or document the term in the notebook's first cell. If it's a one-off demo not slated for blogpost reference, consider moving to examples/_archive/.
• The top-level examples/migration_v5_demo_notebook.ipynb should move into the demo directory after rename: examples/context_graph_demo/context_graph_demo_notebook.ipynb.

7. Pre-blogpost checklist

• All public-API functions have docstrings.
• Type hints on public surface.
• Error messages name the user-facing concept ("ontology binding", "context graph"), not internal model names.
• Log output from bqaa context-graph is structured JSON (already true per periodic-materialization README) and the fields are documented.
• bqaa --help / sub-command help text is curated.
• examples/context_graph_demo/ runs end-to-end on a fresh BQ project with the documented quickstart.
• pyproject.toml [project.urls] points at the right places (adk-docs page, repo, issues).
• CHANGELOG.md notes the rename + design-doc consolidation.

Cleanup plan — proposed wave order

Wave	Scope	Why first / last
1	Source surface audit doc (docs/context_graph_surface_audit.md)	Inputs the rest of the renames
2	Design-doc consolidation (canonical context_graph.md + context_graph_design.md; archive v2)	Settles terminology before user-facing docs change
3	examples/migration_v5/ → examples/context_graph_demo/ rename + all cross-references + 36 path updates	Atomic single PR; touches deploy scripts, tests, generated artifacts, Terraform, notebook
4	Module-level renames (per §4 audit outcomes) — _ontology_routing.py placement, ontology-graph deduplication if any	After audit but independent of the demo rename
5	README / SDK.md re-shape for blogpost narrative	Last so it can reference the new names
6	Pre-blogpost checklist run (docstrings, types, help text, end-to-end smoke)	Final gate

Sub-issues to file

• Wave 1: Write docs/context_graph_surface_audit.md — module inventory, public/internal classification, primary concept per module.
• Wave 2: Consolidate context-graph design docs — docs/context_graph.md (concept), docs/context_graph_design.md (reference), archive v2.
• Wave 3: Rename examples/migration_v5/ → examples/context_graph_demo/ (single atomic PR; 36 paths). Includes the top-level migration_v5_demo_notebook.ipynb moving into the demo directory.
• Wave 3: Decide and apply BQ dataset-name policy (legacy default migration_v5_demo vs. new context_graph_demo; if changed, document the migration step for existing users).
• Wave 3: Update periodic_materialization/ README + deploy scripts + Terraform for the new path. Coordinate timing with the adk-docs page update (companion issue) so external links don't break.
• Wave 4: Module-level audit outcomes — _ontology_routing.py placement; ontology-graph naming deduplication.
• Wave 4: Notebook hygiene — context_graph_adcp_demo.ipynb rename or archive decision.
• Wave 5: README front-page rewrite for blogpost narrative.
• Wave 5: SDK.md split decision (reference + concepts + thin index, or single curated doc with explicit TOC).
• Wave 6: Pre-blogpost checklist completed (docstrings, type hints, error-message review, end-to-end smoke on fresh project, pyproject.toml URLs, CHANGELOG.md entry).

Acceptance

• No migration_v5 path or identifier remains in the repo except inside docs/_archive/ historical material and the CHANGELOG entry that documents the rename.
• A reader landing from a blogpost on README.md can find the context-graph demo in under three clicks and run it against their own BigQuery project from examples/context_graph_demo/README.md.
• docs/context_graph.md is the single canonical concept doc. docs/context_graph_design.md is the single canonical technical reference. v2 lives in _archive/.
• The periodic-materialization deploy works from the renamed path and the adk-docs page (companion issue) points at the new deep links.

Coordination

This work overlaps with the companion adk-docs user-manual issue. The adk-docs page should land after Wave 3 (rename) is merged so the deep links on adk.dev/integrations/bigquery-agent-analytics/ are stable from day one.

References

• examples/migration_v5/README.md — the demo's own description (the pluggable contract and authorship boundary section is the source for the rename rationale).
• examples/migration_v5/periodic_materialization/README.md — production-shape Cloud Run + Cloud Scheduler deploy that will be promoted to user-manual content.
• docs/context_graph_v3_design.md, docs/context_graph_v2_design.md, docs/learning_ontology_and_context_graph.md — the three docs to consolidate.

[caohy1988]

Full review against current main checkout.

Overall: the direction is right, and examples/context_graph_demo/ is still the best rename. I found a few places where the tracker underestimates the real surface or where acceptance should be made more mechanical before sub-issues are filed.

Findings:

1. High: the surface audit omits the separate bigquery_ontology package. The current wheel includes both packages in pyproject.toml:

   • packages = ["src/bigquery_agent_analytics", "src/bigquery_ontology"]
   • console script gm = "bigquery_ontology.cli:main"

   The issue's Wave 1 audit only lists src/bigquery_agent_analytics/ modules. That misses src/bigquery_ontology/*, docs/ontology/*, and the gm CLI, all of which are part of the ontology/context-graph user surface now. Add these to docs/context_graph_surface_audit.md with explicit classification: public package, internal helpers, docs source-of-truth, and relationship to the bqaa context-graph path.

2. High: the canonical-doc consolidation can accidentally conflate two different generations of "context graph." SDK.md still has ## 18. Context Graph (Property Graph for Agentic Ads) built around ContextGraphManager / context_graph.py, while the blogpost/demo path is the ontology-driven materializer (bqaa context-graph, materialize_window.py, ontology_*, generated ontology/binding/property-graph artifacts). The repo also still has docs/ontology_graph_v4_design.md and docs/ontology_graph_v5_design.md listed in docs/README.md. Wave 2 should explicitly decide how these histories relate:

   • old context_graph.py / ContextGraphManager reference kept, deprecated, or archived;
   • ontology graph v4/v5 docs merged into the new canonical context_graph_design.md or deliberately left as archived design history;
   • public wording for "context graph" maps to the production materializer, not the older Agentic Ads-specific API unless that API remains supported.

3. Medium: the migration blast radius should be split into path renames vs. content references. In the current checkout, git ls-files '*migration_v5*' returns the tracked path-name set, while git grep -l migration_v5 finds 31 files with content references. Important non-directory references include docs/blog/periodic_materialization.md, docs/codelabs/periodic_materialization.md, the generated examples/codelab/periodic_materialization/colab_notebook.ipynb, docs/README.md, examples/README.md, CHANGELOG.md, and live/unit tests. Replace the "36 paths" wording with two mechanical acceptance checks:

   • no tracked path contains migration_v5 except archive/changelog if intentionally kept;
   • git grep migration_v5 returns only allowed historical/archive/changelog hits.

4. Medium: the dataset-name decision is broader than generated snapshots. migration_v5_demo is not only in binding.yaml, table_ddl.sql, and property_graph.sql; it is also the default in examples/migration_v5/mako_artifacts.py, examples/migration_v5/mako_demo_agent.py, APP_NAME, tests such as tests/test_migration_v5_ontology_artifacts.py, tests/test_materialize_window_live.py, and tests/test_ontology_property_graph.py. Terraform also defaults graph_dataset_id = "migration_v5_graph". If the goal is blogpost-facing polish, I would pick the new default context_graph_demo for fresh runs and document the old dataset name only as legacy/migration history. Keeping migration_v5_demo as the default conflicts with the issue's own "no migration_v5 identifier remains" acceptance.

5. Medium: CLI/backward-compat surface needs its own sub-issue. The current CLI already has product vocabulary: bqaa context-graph is registered as the preferred command, while bq-agent-sdk materialize-window and the bqaa-materialize-window console script remain in pyproject.toml. Wave 5/6 should explicitly decide whether these are documented aliases, deprecated aliases, or hidden compatibility shims. Otherwise the docs rename can land while --help, package entry points, and old command names still send users to the pre-blogpost mental model.

6. Low: update generated codelab artifacts as part of Wave 3. The workflow references docs/codelabs/periodic_materialization.md and examples/codelab/periodic_materialization/colab_notebook.ipynb; the notebook contains old deep links to examples/migration_v5/periodic_materialization. Add "regenerate/check the codelab notebook" to the Wave 3 acceptance so the Markdown and generated .ipynb do not drift.

Suggested extra acceptance:

• pyproject.toml has [project.urls] filled in before the blogpost; it currently has none.
• bqaa context-graph --help, bq-agent-sdk materialize-window --help, and bqaa-materialize-window --help are reviewed together for naming/deprecation language.
• docs/context_graph_surface_audit.md covers src/bigquery_agent_analytics, src/bigquery_ontology, docs/ontology, the bqaa CLI, and generated/demo artifacts.

[caohy1988]

Follow-up after incorporating #277 / PRs #278-#281.

#277 is now merged through PR #281, so the cleanup plan should treat the one-artifact context-graph flow as current main, not future design:

• feat(ontology): offline CREATE PROPERTY GRAPH DDL parser (issue #277, PR 1) #278: DDL parser
• feat(ontology): join parsed graph with table schemas for property types (issue #277, PR 2) #279: table-schema type join
• feat(ontology): synthesize ontology+binding from a property graph (issue #277, PR 3) #280: ontology+binding synthesis
• feat(cli): bqaa context-graph --property-graph derives ontology+binding (issue #277, PR 4) #281: bqaa context-graph --property-graph property_graph.sql, with --ontology/--binding kept as the explicit override mode

That changes #282 in a few concrete ways:

1. Wave 1 surface audit should include the new Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 modules. Add src/bigquery_agent_analytics/property_graph_spec.py and the src/bigquery_ontology/{graph_ddl_parser.py,graph_schema_join.py,graph_to_spec.py} pipeline to docs/context_graph_surface_audit.md. These are now the product path for bqaa context-graph, not just internal helpers.

2. Wave 3 rename should also migrate the periodic deploy to property-graph mode. Current examples/migration_v5/periodic_materialization/run_job.py still retargets binding.yaml and calls run_materialize_window(... ontology_path=..., binding_path=...). deploy_cloud_run_job.sh still stages ontology.yaml + binding.yaml. After Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277, the blogpost/demo path should stage/use property_graph.sql as the primary input and reserve ontology/binding for "advanced override / generated reference" content. Otherwise the renamed demo still teaches the pre-Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 four-artifact workflow.

3. The dataset-name decision now includes placeholder substitution in property_graph.sql. property_graph_spec.derive_ontology_binding_from_ddl() substitutes ${PROJECT_ID} / ${DATASET} before parsing. If the demo snapshots move to context_graph_demo, prefer making the checked-in property_graph.sql placeholder-based where possible, rather than baking test-project-0728-467323.migration_v5_demo into the primary docs. The YAML snapshots can remain generated/reference artifacts if needed, but the default user path should not depend on editing them.

4. Design-doc consolidation should change the artifact story. The current issue says the pluggable contract has four generated artifacts (ontology.yaml / binding.yaml / table_ddl.sql / property_graph.sql). After Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277, the user-authored/public contract should be: table DDL + property-graph DDL, with ontology/binding derived internally. Ontology/binding are still valuable as advanced overrides and as generated/debug artifacts, but they should not be framed as required for the main blogpost path.

5. Acceptance should add a Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 regression gate. Suggested mechanical gate:

   • bqaa context-graph --property-graph examples/context_graph_demo/property_graph.sql ... works with no --ontology / --binding;
   • periodic Cloud Run local dry-run uses the same property-graph input mode;
   • old YAML mode is tested/documented as an override, not the default recipe.

Net: #277 strengthens the cleanup story, but only if #282 makes it the primary demo/manual path. Otherwise the repo rename lands while the production example still teaches the older ontology+binding-required workflow.

[caohy1988]

Full review of the #277 incorporation comment

Verified every claim in the prior follow-up against current main. All correct. A few additional gaps + concrete body-edit recommendations.

Claims confirmed against source

Claim	Verified at
bqaa context-graph (materializer) now accepts --property-graph	src/bigquery_agent_analytics/cli.py:1773-1797, flag declared at :1817-1819, mutual-exclusion checks at :1783-1791
Periodic deploy still uses YAML mode	examples/migration_v5/periodic_materialization/run_job.py:479,499-503 (still calls _retarget_binding and passes ontology_path + binding_path)
deploy_cloud_run_job.sh still stages YAML	lines 56, 569, 589-590 still copy ontology.yaml + binding.yaml into the staging tree
src/bigquery_ontology/ carries the #277 derivation modules	graph_ddl_parser.py, graph_schema_join.py, graph_to_spec.py, graph_ddl_compiler.py, graph_ddl_models.py all present
src/bigquery_agent_analytics/property_graph_spec.py is the public API for derivation	4.7 KB, present

Additional gaps the follow-up did not cover

1. Both code paths are wired into the same materialize_window command (cli.py:1797-2046). The mutual-exclusion error at cli.py:1791 currently reads: "Provide --property-graph PATH, or both --ontology PATH and binding PATH". The cleanup should flip the order of the two paths in every user-facing surface — CLI help, error messages, README quickstart, periodic-materializer recipe — so --property-graph is presented first. Same flip in cli.py:1817-1819 (--property-graph is already listed first in the Typer.Option list — good; but the error string above needs reordering).

2. There are two CLIs in the repo that the surface audit must classify together:

   • src/bigquery_agent_analytics/cli.py — product CLI (bqaa context-graph, bqaa init, etc.)
   • src/bigquery_ontology/cli.py (33.5 KB) — ontology authoring CLI (commands: validate, compile, scaffold, import-owl)

   Open decision for Wave 1 / Wave 4: should the ontology CLI become a bqaa ontology subcommand group under the product CLI (one entry point, two groups), or stay separate as an "advanced authoring tool"? For a blogpost-aligned story, folding it under bqaa ontology is cleaner: one binary, primary context-graph workflow, advanced ontology group for TTL authoring.

3. examples/migration_v5/ontology_artifacts.py (29.3 KB) reverse-migration story. Post-Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 there are two TTL/DDL ↔ semantic-file pipelines:

   • TTL → ontology+binding YAML via ontology_artifacts.py (existing, pre-Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277)
   • property_graph.sql + live INFORMATION_SCHEMA → in-memory Ontology + Binding via property_graph_spec.derive_ontology_binding_from_ddl() (Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277)

   The cleanup needs to position both honestly. Recommendation: property_graph.sql is the primary user input; ontology_artifacts.py becomes an advanced authoring path for users who already have an OWL TTL and want to drive both files + the property-graph from it. Mention this explicitly in docs/context_graph.md after Wave 2.

4. The dataset-name decision is tied to placeholder substitution in the checked-in property_graph.sql. Today examples/migration_v5/binding.yaml bakes test-project-0728-467323.migration_v5_demo as a literal. Post-Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277, property_graph_spec.derive_ontology_binding_from_ddl() performs ${PROJECT_ID} / ${DATASET} substitution before parsing. Concrete Wave 3 task: the checked-in property_graph.sql should use placeholder syntax, not literal dataset names, so the rename to context_graph_demo does not require regenerating it. The binding.yaml literal becomes irrelevant in the primary path.

5. The 36-paths count is still 36 post-Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 — verified just now. No drift from when Tracking: SDK production cleanup — rename migration_v5, consolidate context-graph surface, blogpost-ready audit #282 was filed; Wave 3 sizing is accurate.

Recommended body edits

Apply these to the issue body:

• §Audit findings Add streaming evaluation, Dashboard V2, design docs, and CI workflow #1 (the migration_v5 name): keep the table of candidates as-is, but add a one-line note that context_graph_demo/ is also the name the blogpost will use externally per Tracking: expand adk.dev/integrations/bigquery-agent-analytics/ into a full user manual #283.
• §Audit findings Overhaul README, add documentation indexes, and fix CI issues #4 (Source surface audit): extend the file list to include src/bigquery_ontology/{graph_ddl_parser,graph_schema_join,graph_to_spec,graph_ddl_compiler,graph_ddl_models,binding_loader,binding_models,ontology_loader,ontology_models,owl_importer,scaffold,concept_index,_fingerprint,cli}.py and src/bigquery_agent_analytics/property_graph_spec.py. The audit doc should classify both the existing and the Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277 modules in one pass.
• New §Audit finding Move ontology reference documentation to docs/ontology/ #8 — Two CLIs: capture the bqaa ontology subcommand-group consolidation question above.
• §Cleanup plan / Wave 3: add a sub-task "migrate periodic_materialization/run_job.py and deploy_cloud_run_job.sh from YAML mode to --property-graph mode" — drop YAML retargeting in run_job.py:479,499-503; drop YAML staging in deploy_cloud_run_job.sh:56,569,589-590; pass --property-graph property_graph.sql to the materializer; rely on derive_ontology_binding_from_ddl() for placeholder substitution. The directory rename alone leaves the older workflow in place — the user-facing fix is the property-graph migration, the rename is the namespace fix.
• §Cleanup plan / Wave 3: add a sub-task "checked-in property_graph.sql uses ${PROJECT_ID} / ${DATASET} placeholders, not the demo's hardcoded test-project-0728-467323.migration_v5_demo".
• §Cleanup plan / Wave 5: add "flip CLI help / error-message ordering at cli.py:1791 so --property-graph is presented first; this is the user-visible cue that the property-graph path is primary".
• §Acceptance: add "bqaa context-graph --property-graph examples/context_graph_demo/property_graph.sql --project-id ... --dataset-id ... succeeds with no --ontology / --binding flags. The periodic Cloud Run smoke (./deploy_cloud_run_job.sh --smoke ...) uses the same input mode. Legacy YAML mode is documented as an advanced override, not the primary path."

Net

The #277 incorporation comment correctly identifies that the rename alone is insufficient. The five items above are the concrete code/script paths and CLI-level details to add so Wave 3 actually delivers the new workflow rather than just renaming the directory around the old one.

[caohy1988]

Fresh pass after the #277 incorporation review, focusing on the actual post-#281 code paths.

The prior reviews still stand. I found one additional blocker that should be added before Wave 3 is treated as implementation-ready.

New finding

1. High: --property-graph mode currently has a single dataset_id, but the periodic deploy has split events and graph datasets.

   The periodic job intentionally reads raw events from BQAA_EVENTS_DATASET_ID and writes graph tables/state into BQAA_GRAPH_DATASET_ID. Current code:

   • examples/migration_v5/periodic_materialization/run_job.py calls run_materialize_window(dataset_id=events_dataset_id, ...), then uses a retargeted YAML binding.yaml to point entity/relationship writes at graph_dataset_id.
   • src/bigquery_agent_analytics/materialize_window.py::_resolve_ontology_binding() passes that same dataset_id into derive_ontology_binding_from_ddl(...).
   • src/bigquery_agent_analytics/property_graph_spec.py uses dataset_id for ${DATASET}, the BigQuerySchemaProvider default dataset, and derive_ontology_binding(... dataset=dataset_id).

   That is fine for the codelab single-dataset shape, but it is not sufficient for the production periodic-materializer shape unless every graph table in property_graph.sql is fully qualified with the graph dataset and the binding target/default semantics are explicitly handled. The current tests also only cover same-dataset derivation (tests/test_property_graph_spec.py asserts binding.target.dataset == "d").

   Concrete Wave 3 / Feat: derive ontology + binding from the property-graph DDL so bqaa context-graph no longer requires ontology.yaml/binding.yaml #277-follow-up task: add an explicit graph-dataset input to the property-graph derivation path before migrating run_job.py:

   • either run_materialize_window(..., property_graph_dataset_id=...) / CLI option / env var, or a clearly named substitution contract such as ${GRAPH_DATASET} / ${BQAA_GRAPH_DATASET_ID};
   • property_graph.sql for the demo should use graph-dataset placeholders, not ${DATASET} if ${DATASET} continues to mean the events dataset;
   • tests must cover events_dataset != graph_dataset, including derived binding target, schema lookup, materializer writes, state table, and state-key fingerprint stability.

   Without this, the Wave 3 instruction "migrate periodic_materialization to --property-graph mode" is under-specified and can regress the production split-dataset contract that the README currently documents.

2. Medium: revise the placeholder recommendation from ${DATASET} to graph-specific placeholders.

   The earlier follow-up recommended placeholder-based property_graph.sql, which is still right. But given the split dataset model, the placeholder should be graph-specific. ${DATASET} is currently bound by property_graph_spec.py to the materializer's dataset_id, which is the events dataset in the periodic job. Use ${GRAPH_DATASET} / ${BQAA_GRAPH_DATASET_ID} (or add a first-class graph dataset option) for graph tables.

3. Medium: derived mode + default ai-fallback has no descriptions.

   bigquery_ontology.graph_to_spec correctly documents that descriptions/synonyms are not recoverable from DDL/schema. The CLI default remains --extraction-mode=ai-fallback, so a primary property-graph-mode recipe can still call AI.GENERATE with less guided prompts than the explicit ontology YAML path. That is acceptable if documented, but Wave 5/6 should make the production recommendation explicit:

   • default property-graph mode is the simplest path;
   • compiled-only is the deterministic production path when reference extractors exist;
   • explicit ontology/binding remains the advanced override when prompt descriptions/logical names matter.

Net: #277 is still the right direction, but Wave 3 needs a split-dataset regression gate before the periodic Cloud Run recipe can safely move from YAML mode to property-graph mode.

[caohy1988]

Validation + additional gaps on the split-dataset finding

Finding #1 verified on every code path; the rest of the addendum follows from it. Additional gaps below.

Validation

Claim	Verified at
run_job.py reads from events dataset, writes to graph dataset	run_job.py:370-371 (env vars), :46-49 (docstring), :479,499-503 (_retarget_binding(...) + run_materialize_window(dataset_id=events_dataset_id, ...))
YAML mode handles split via _retarget_binding()	run_job.py:225-249 — swaps binding["target"]["dataset"] = graph_dataset_id AND rewrites each entity/relationship's source from {project}.{events}.{table} to {project}.{graph_dataset}.{table}
--property-graph mode collapses on a single dataset_id	property_graph_spec.py::derive_ontology_binding_from_ddl() — dataset_id is used three ways: ${DATASET} substitution, BigQuerySchemaProvider(default_dataset=dataset_id), and derive_ontology_binding(... dataset=dataset_id)
run_materialize_window has no separate graph-dataset arg	materialize_window.py:1282-1291 — single dataset_id; _resolve_ontology_binding at :1233-1279 passes it unchanged into property-graph mode
Tests don't cover split	The addendum cites tests/test_property_graph_spec.py asserting binding.target.dataset == "d"

A naive run_job.py migration that just swaps ontology_path / binding_path for property_graph_path would derive a binding with target.dataset = events_dataset_id and write graph tables into the events dataset. Silent correctness regression. The addendum's gate-before-Wave-3 framing is right.

Additional gaps the addendum did not cover

1. The schema-lookup direction also needs to land on the graph dataset. BigQuerySchemaProvider(default_project=project_id, default_dataset=dataset_id) is used to recover property types from INFORMATION_SCHEMA.COLUMNS. The DDL's FROM clauses point at the graph tables (entity / relationship). If dataset_id continues to mean events, the schema lookup will read events_dataset.INFORMATION_SCHEMA.COLUMNS and fail to find the graph tables. So the new graph_dataset_id parameter (whichever shape the API lands in) is needed both for the binding target and for the schema provider's default.

2. run_materialize_window API choice is the real design question, not just "add a graph dataset arg". Three shapes worth considering:

   Shape	Pro	Con
   Add graph_dataset_id: Optional[str] = None (defaults to dataset_id)	Back-compat with codelab single-dataset	Implicit; codelab and prod look the same from the API surface
   Rename dataset_id → events_dataset_id, add graph_dataset_id: Optional[str] = None	Explicit; codelab can omit graph	Breaking API change; ripple through cli.py flags
   Single dataset_id + new binding_target_dataset_id: Optional[str]	Minimal semantic change	Awkward name; doesn't cover schema-lookup direction

   Recommended: shape Revamp README, enhance documentation navigation, and fix CI #2. Renaming dataset_id → events_dataset_id is breaking but reads correctly and matches the env-var names already used in run_job.py. The migration window aligns with the rest of Tracking: SDK production cleanup — rename migration_v5, consolidate context-graph surface, blogpost-ready audit #282 cleanup, so the breaking change has a natural batching slot.

3. The state table is unaffected. _bqaa_materialization_state is constructed explicitly at run_job.py:491 as f"{project_id}.{graph_dataset_id}._bqaa_materialization_state" — separate from the derived binding. The split-dataset regression test should verify this stays in graph_dataset, but it doesn't need code changes.

4. Substitution-name decision affects the demo's property_graph.sql shape. Two clean conventions:

   • Codelab uses ${DATASET} (single). The demo's property_graph.sql reads cleanly for the simplest shape; the periodic-deploy version is a separate file with ${GRAPH_DATASET}.
   • Single SQL file with ${GRAPH_DATASET} everywhere. ${DATASET} aliases to ${GRAPH_DATASET} for codelab, but the demo authoritatively uses the graph-specific name. One SQL file works for both shapes.

   Recommended: shape B. Avoids maintaining two snapshots and makes the production shape the default mental model.

5. _retarget_binding() is dead code in property-graph mode. Once run_job.py migrates, lines 225-249 (the binding-retargeting helper) become unreachable from the primary path. Wave 3 should delete it rather than leaving it as legacy reachable-only-via-YAML-mode code. Mention _retarget_binding() explicitly in the Wave 3 sub-task list.

6. tests/test_property_graph_spec.py regression suite needs four cases, not one:

   • events_dataset == graph_dataset (codelab shape — current test).
   • events_dataset != graph_dataset, ${GRAPH_DATASET} placeholders in DDL.
   • events_dataset != graph_dataset, fully-qualified table refs in DDL (no placeholders).
   • End-to-end periodic-materializer smoke against the post-rename examples/context_graph_demo/ with split datasets.

Recommended body edits

Add to #282:

• §Cleanup plan / Wave 3 sub-task: "Add graph_dataset_id to the property-graph derivation path (run_materialize_window, _resolve_ontology_binding, derive_ontology_binding_from_ddl, BigQuerySchemaProvider) before migrating run_job.py. Recommended API rename: dataset_id → events_dataset_id, new graph_dataset_id: Optional[str] = None. Update CLI flags accordingly."
• §Cleanup plan / Wave 3 sub-task: "Delete _retarget_binding() at run_job.py:225-249 once run_job.py uses --property-graph mode."
• §Cleanup plan / Wave 3 sub-task: "Add ${GRAPH_DATASET} substitution to property_graph_spec.resolve_placeholders and the demo's property_graph.sql. Decide on ${DATASET} aliasing (recommended: keep as an alias of ${GRAPH_DATASET} for codelab compatibility)."
• §Acceptance (new bullet): "tests/test_property_graph_spec.py covers events_dataset != graph_dataset with both ${GRAPH_DATASET} placeholders and fully-qualified table refs. Periodic Cloud Run smoke (./deploy_cloud_run_job.sh --smoke ...) runs with --property-graph and split datasets, with entity/relationship rows written to graph dataset and state table at {project}.{graph}._bqaa_materialization_state."

Net

The split-dataset finding is the right last blocker. The API choice — whether to rename dataset_id to events_dataset_id or layer graph_dataset_id on top — is the design question to answer in Wave 3 before the run_job.py migration. Without it, the Wave 3 migration silently regresses production behaviour even with #277 fully merged.

[caohy1988]

Fresh pass after the split-dataset validation comment.

The split-dataset finding is correct. I would adjust the implementation recommendation in two places before Wave 3 is treated as ready.

Findings

1. High: avoid making dataset_id an immediate breaking rename unless this is deliberately a major-version cleanup.

   The current public-ish Python API and CLI surface already uses run_materialize_window(..., dataset_id=...), and the code currently interprets it as the events-table dataset:

   • events_table_ref = validated_table_ref(project_id, dataset_id, events_table)
   • state-table default also uses dataset_id unless the caller passes a fully qualified state_table
   • the CLI/docs/codelab already teach --dataset-id

   The prior recommendation was dataset_id -> events_dataset_id plus graph_dataset_id. That is the clearest long-term naming, but it is a breaking change across the SDK, CLI examples, codelab, tests, and any users calling the function directly.

   Recommended Wave 3 implementation shape:

   • keep dataset_id as the backward-compatible events dataset;
   • add graph_dataset_id: Optional[str] = None, defaulting to dataset_id;
   • thread graph_dataset_id into _resolve_ontology_binding() / derive_ontology_binding_from_ddl() for ${GRAPH_DATASET}, schema lookup defaults, and binding target;
   • CLI adds --graph-dataset-id (or env-backed equivalent) without renaming --dataset-id in the same PR;
   • docs can say --dataset-id = events dataset, --graph-dataset-id = graph/write dataset;
   • optional follow-up: add events_dataset_id as a clearer alias and deprecate ambiguous dataset_id over time.

   Acceptance should include a test proving existing run_materialize_window(dataset_id="d", property_graph_path=...) still works for the single-dataset/codelab case.

2. High: Wave 3 needs an explicit state-key migration decision.

   The state table location is unaffected when run_job.py passes a fully qualified graph-dataset state table, but the state key can still change. compute_state_key() includes dataset_id, graph_name, events_table, ontology_fingerprint, and binding_fingerprint. Moving the periodic job from YAML-retargeted binding mode to property-graph-derived binding mode can change the ontology/binding fingerprints even when the logical graph is the same.

   That means the first post-migration scheduled run may start a new checkpoint namespace and reprocess the lookback window instead of continuing from the old YAML-mode checkpoint. That may be acceptable, but it needs to be intentional.

   Add Wave 3 acceptance:

   • document whether YAML-mode -> property-graph-mode migration intentionally creates a new state_key;
   • dry-run output/logs expose enough information for operators to see the new key/fingerprint inputs;
   • periodic README/adk-docs explain first-run behavior after the migration: expected reprocessing window, duplicate/idempotent write expectations, and when to use a backfill/suffix instead of relying on the steady checkpoint.

3. Medium: ${DATASET} as an alias is useful but dangerous in split mode.

   If --dataset-id continues to mean events dataset, then ${DATASET} in property_graph.sql cannot also silently mean graph dataset without creating a documentation trap. Prefer first-class names:

   • ${EVENTS_DATASET} for telemetry reads, if ever needed in SQL snippets;
   • ${GRAPH_DATASET} for graph table refs;
   • keep ${DATASET} only as a single-dataset/codelab compatibility alias, or make it explicitly alias ${GRAPH_DATASET} with docs that say so.

   Add a regression test that split mode with ${GRAPH_DATASET} produces binding.target.dataset == graph_dataset_id and schema lookup reads graph table metadata.

4. Medium: fully qualified table refs do not remove the need for an explicit graph target.

   The previous test recommendation for fully qualified refs is right, but there is one more invariant to pin: even when every DDL table ref is fully qualified, the binding target still controls default write/resolve behavior elsewhere in the materializer. So the derived binding target should still be graph_dataset_id, not inferred from dataset_id and not guessed from the first table ref.

   If a property graph references tables across multiple datasets, require explicit graph_dataset_id and document that it is the materialized output/default target. Do not infer it from the refs.

Recommended body edits

• Change the API recommendation from an immediate dataset_id -> events_dataset_id breaking rename to a compatibility-preserving shape: keep dataset_id, add graph_dataset_id, optionally introduce events_dataset_id as a future clearer alias.
• Add a Wave 3 sub-task: "Decide and document state-key behavior for YAML-mode -> property-graph-mode migration."
• Add acceptance for the old single-dataset call signature, split ${GRAPH_DATASET} placeholders, fully qualified refs, and the state-key migration story.

Net: the split-dataset blocker is real, but the lowest-risk cleanup path is additive API support plus explicit docs, not an immediate rename of the existing dataset parameter.

[caohy1988]

Endorsement + additional gaps

Reversing my prior API-rename recommendation in favor of yours. The additive shape is the right call, and the state-key finding is exact on the current code. Three additional gaps worth surfacing.

Validation

Claim	Verified at
state_key includes ontology+binding fingerprints	materialize_window.py:36-38 docstring: "sha256 of project + dataset + graph_name + events_table + ontology_fingerprint + binding_fingerprint + discovery_mode". YAML→derived path changes 2 of 7 inputs.
dataset_id is the events-table dataset by current convention	materialize_window.py:341,384,405-428 (validated_table_ref(project_id, dataset_id, events_table)), :458,481 (FROM {events_table_ref_quoted} in events SELECTs)
State-table default location derives from dataset_id unless caller passes fully-qualified state_table	confirmed; run_job.py:491 already passes a fully-qualified f"{project_id}.{graph_dataset_id}._bqaa_materialization_state" so the state-table location itself stays on graph_dataset post-migration
Existing checkpoint-namespace invalidation pattern	materialize_window.py:40-44: "A config change — including a swap of --completion-event-type or a debug mode flip — auto-invalidates the previous checkpoint so the new predicate's run cannot inherit the old predicate's high-water mark." — exactly the mechanism that handles the YAML→property-graph migration

I'm reversing my prior dataset_id -> events_dataset_id rename recommendation. The additive shape (keep dataset_id, add graph_dataset_id: Optional[str] = None defaulting to dataset_id) is lower-risk and reads correctly because the events-dataset semantic is already the de-facto interpretation in every site that uses dataset_id today.

Additional gaps

1. Frame the first-run state-key reset as a feature, not a migration corner case. Lines 40-44 of materialize_window.py already document the contract: any semantic change to the predicate, binding, or ontology auto-invalidates the previous checkpoint. The YAML→property-graph migration is exactly one instance of that contract — not a new failure mode. Wave 3 acceptance should call this out so operators read it as expected behavior governed by the existing state-key invariant, not as a YAML-vs-derived idiosyncrasy. Suggested wording for the README and CHANGELOG.md:

   Migrating the periodic deploy from --ontology+--binding to --property-graph changes ontology_fingerprint and binding_fingerprint, which are two of the seven inputs to the materializer's state_key. The first post-migration run will start a new checkpoint namespace and reprocess the configured lookback_hours window. This is the same mechanism that protects against --completion-event-type swaps and debug-mode flips; per-session writes remain idempotent.

2. derive_ontology_binding_from_ddl() should accept both kwargs and explicit precedence. The signature should look like:

   def derive_ontology_binding_from_ddl(
       ddl_text: str,
       *,
       project_id: str,
       dataset_id: str,                          # events dataset (current convention)
       graph_dataset_id: Optional[str] = None,   # defaults to dataset_id
       bq_client: Any,
       substitutions: Optional[Mapping[str, str]] = None,
       metadata_columns: Sequence[str] = _DEFAULT_METADATA_COLUMNS,
   ): ...

   Inside, the substitution map binds ${DATASET} to dataset_id for back-compat, ${EVENTS_DATASET} to dataset_id, and ${GRAPH_DATASET} to graph_dataset_id or dataset_id. The BigQuerySchemaProvider default_dataset and the derived binding's target.dataset both use graph_dataset_id or dataset_id. Codelab call sites that pass only dataset_id keep working unchanged; production sites pass graph_dataset_id= explicitly.

3. IAM matrix on the graph dataset is incomplete for the schema-lookup path. BigQuerySchemaProvider reads INFORMATION_SCHEMA.COLUMNS on the graph dataset (not the events dataset). The current deploy_cloud_run_job.sh IAM grants roles/bigquery.dataEditor on the graph dataset for entity/relationship writes; that covers data reads/writes but does not explicitly cover INFORMATION_SCHEMA.COLUMNS (which is a metadata read). In practice dataEditor grants metadata-read access too, so this is not a runtime blocker — but the periodic-deploy IAM matrix in the README and deploy_cloud_run_job.sh should call out the metadata-read requirement explicitly so operators downgrading to dataViewer + jobUser would know they need a metadata-viewer role on top.

Recommended body edits

• §Audit findings / API shape: replace the breaking-rename language with: "Recommended additive shape — keep dataset_id as the events dataset; add graph_dataset_id: Optional[str] = None (defaults to dataset_id); thread through _resolve_ontology_binding → derive_ontology_binding_from_ddl (substitution map + BigQuerySchemaProvider.default_dataset + binding target). CLI adds --graph-dataset-id without renaming --dataset-id. Optional follow-up after blogpost: introduce events_dataset_id as a clearer alias and deprecate ambiguous dataset_id."
• §Wave 3 acceptance add: "Test: run_materialize_window(dataset_id='d', property_graph_path=...) succeeds with graph_dataset_id=None (codelab compat). Test: split-dataset case with ${GRAPH_DATASET} placeholders. Test: split-dataset case with fully-qualified table refs in DDL. Test: first-run checkpoint behaviour — moving from YAML to property-graph mode on the same data produces a different state_key (semantic-change invariant)."
• §Wave 3 sub-task: "Document the state-key invalidation note in examples/context_graph_demo/periodic_materialization/README.md and CHANGELOG.md. Frame as 'expected behaviour of the existing state-key invariant,' not as a YAML-vs-property-graph quirk."
• §Wave 3 sub-task: "Update deploy_cloud_run_job.sh IAM matrix comment block (lines ~292-525) to call out the metadata-read requirement on the graph dataset for INFORMATION_SCHEMA.COLUMNS."
• §Placeholder convention: property_graph_spec.resolve_placeholders substitutes ${PROJECT_ID}, ${EVENTS_DATASET}, ${GRAPH_DATASET}. Keep ${DATASET} as an alias of ${GRAPH_DATASET} for codelab compat, documented as such.

Net

The state-key concern is exactly the mechanism materialize_window.py:40-44 already documents as protective, so the migration story is "by-design, governed by the existing state-key invariant." The additive API shape lets the codelab keep working unchanged while production gains the split-dataset surface. The IAM-metadata-read note is the smallest open item — worth one line in the deploy-script comment block.

[caohy1988]

Fresh pass after the additive graph_dataset_id endorsement.

The current direction is right. I found a few precision issues that should be fixed before the body/sub-issues are updated.

Findings

1. High: the placeholder contract currently contradicts itself on ${DATASET}.

   The latest comment has two different contracts:

   • signature sketch: ${DATASET} -> dataset_id (events dataset)
   • recommended body edit: ${DATASET} -> alias of ${GRAPH_DATASET} for codelab compatibility

   The second one is the safer contract for property-graph DDL. property_graph.sql table refs are graph-table refs, not agent_events refs. In split mode, mapping ${DATASET} to the events dataset would recreate the original bug under a legacy placeholder name.

   Recommended invariant:

   effective_graph_dataset = graph_dataset_id or dataset_id
   mapping["PROJECT_ID"] = project_id
   mapping["EVENTS_DATASET"] = dataset_id
   mapping["GRAPH_DATASET"] = effective_graph_dataset
   mapping["DATASET"] = effective_graph_dataset  # legacy/codelab alias

   Then BigQuerySchemaProvider(default_dataset=effective_graph_dataset) and derive_ontology_binding(... dataset=effective_graph_dataset) use the same value. Add a test that split mode with legacy ${DATASET} resolves to graph dataset, not events dataset, so old codelab-style SQL does not become dangerous in production.

2. High: the default state-table dataset needs to follow the graph/write dataset when graph_dataset_id is supplied.

   Today run_materialize_window() defaults state_table through:

   parse_state_table_ref(
       state_table or DEFAULT_STATE_TABLE_NAME,
       default_project=project_id,
       default_dataset=dataset_id,
   )

   That is correct before graph_dataset_id exists, and run_job.py masks it by passing a fully qualified graph-dataset state table. But once the public CLI supports --graph-dataset-id, a direct CLI/API caller who omits --state-table would still create/read _bqaa_materialization_state in the events dataset unless the default changes.

   Recommended contract:

   • events_table_ref uses dataset_id;
   • binding target / schema lookup / materialized tables use effective_graph_dataset;
   • default state_table also uses effective_graph_dataset;
   • an explicit state_table still wins.

   Add acceptance that run_materialize_window(dataset_id="events", graph_dataset_id="graph", state_table=None, ...) resolves the state table to {project}.graph._bqaa_materialization_state.

3. Medium: do not make the state-key test assert "YAML -> property-graph always changes the key."

   The invariant is: a fingerprint change changes the state key. The mode switch itself is not the key input. If the derived ontology/binding is intentionally made fingerprint-equivalent to the committed YAML in a future implementation, reusing the same state key would be valid.

   Better acceptance:

   • expose/log the ontology and binding fingerprints in dry-run output;
   • unit-test compute_state_key() changes when ontology or binding fingerprint changes;
   • migration docs say "the first property-graph-mode run starts a new checkpoint namespace if the derived fingerprints differ from the prior YAML fingerprints"; if current MAKO artifacts are known to differ, say that specifically for this demo.

   This keeps the docs tied to the existing state-key invariant without baking in an unnecessary mode-level assertion.

4. Medium: define substitution precedence explicitly.

   derive_ontology_binding_from_ddl() currently starts from os.environ, then binds PROJECT_ID / DATASET, then applies substitutions. With new graph placeholders, the precedence matters. The API/CLI-provided project_id, dataset_id, and graph_dataset_id should not be accidentally shadowed by ambient environment variables.

   Recommended documented precedence:

   1. environment provides optional extra placeholders;
   2. explicit API/CLI args set PROJECT_ID, EVENTS_DATASET, GRAPH_DATASET, and DATASET;
   3. substitutions may override only if the caller deliberately passes them.

   Add a unit test where the process environment contains a conflicting GRAPH_DATASET, and the explicit graph_dataset_id still wins unless substitutions={"GRAPH_DATASET": ...} is passed.

Recommended body edits

• Resolve ${DATASET} to the effective graph dataset, not the events dataset.
• Add state_table defaulting to the graph dataset when graph_dataset_id is present.
• Reword the state-key acceptance from "YAML -> property-graph produces a different key" to "fingerprint changes produce a different key; this migration may/will do so depending on the actual derived fingerprints."
• Add substitution-precedence tests.

Net: additive graph_dataset_id is still the right API shape. The remaining blockers are making the old ${DATASET} placeholder safe in split mode and making the state-table default follow the new graph/write target.

[caohy1988]

Endorsement + concession + additional gaps

All four findings endorsed. Acknowledging the self-contradiction in my prior comment — the safer aliasing for ${DATASET} is the graph-dataset path, not the events-dataset path. Source citations + a few additional gaps.

Validation

Finding	Verified at
#1 ${DATASET} should alias graph, not events	property_graph_spec.py:78-79 docstring says ${DATASET} is "the binding target and the default dataset for unqualified table references" — both of which must be the graph dataset in split mode. Property-graph DDL's FROM clauses point at entity/relationship tables (graph), not agent_events. Aliasing ${DATASET} to events would silently rewrite the schema-lookup target and break codelab SQL the moment a user adds --graph-dataset-id.
#2 state-table default needs to follow graph	materialize_window.py:95 DEFAULT_STATE_TABLE_NAME = "_bqaa_materialization_state"; :431-432 parse_state_table_ref(raw, default_project, default_dataset). Today run_materialize_window passes default_dataset=dataset_id — fine before graph_dataset_id. After, a CLI caller who omits --state-table would silently create {project}.{events_dataset}._bqaa_materialization_state. run_job.py:491 masks this in the periodic deploy with a fully-qualified state table, but the CLI default would still be wrong.
#3 fingerprint-conditional state-key wording	materialize_window.py:36-38 composition; :382-385 components = [project_id, dataset_id, graph_name, ...]. The invariant is fingerprint-based, not mode-based.
#4 substitution precedence	property_graph_spec.py:99-103: dict(os.environ) → mapping["PROJECT_ID"] / mapping["DATASET"] → if substitutions: mapping.update(substitutions). Current code matches the proposed precedence (env < API args < substitutions); but it's only implicit. The docstring at :82-83 says substitutions "override the environment and the PROJECT_ID / DATASET defaults" — partial documentation. Worth making the precedence explicit with a unit test for the env-shadow case.

Conceding the prior ${DATASET} contradiction

My prior comment had a code-block comment saying "events dataset (current convention)" and a body-edit recommendation saying "Keep ${DATASET} as an alias of ${GRAPH_DATASET} for codelab compat". Those are mutually exclusive. The aliasing-to-graph contract is the right one because:

• DDL table refs are graph tables, full stop. ${DATASET} resolving to events would silently mistarget every unqualified ref.
• Existing codelab property_graph.sql snippets that use ${DATASET} (single-dataset shape: events == graph) keep working unchanged in single-dataset mode and gain correct semantics in split mode.
• The events dataset has no role in DDL substitution — it only appears in the materializer's runtime SELECTs against agent_events. If a future SQL surface ever needs an events-dataset placeholder, ${EVENTS_DATASET} is the explicit name.

So the final mapping is:

effective_graph_dataset = graph_dataset_id or dataset_id
mapping: dict[str, str] = dict(os.environ)
mapping["PROJECT_ID"]     = project_id
mapping["EVENTS_DATASET"] = dataset_id
mapping["GRAPH_DATASET"]  = effective_graph_dataset
mapping["DATASET"]        = effective_graph_dataset  # legacy / codelab alias
if substitutions:
  mapping.update(substitutions)

Additional gaps

1. compute_state_key() should keep using dataset_id (events), not graph_dataset_id. materialize_window.py:382-385 already includes dataset_id as a key component. If the state-key composition switched to using effective_graph_dataset, every single-dataset deploy's state-key would change the day they upgrade — even without setting --graph-dataset-id. The conservative move is to keep the state-key composition exactly as-is (dataset_id = events). Net effect: adding graph_dataset_id does not rotate the state-key by itself; only ontology/binding fingerprint changes do. This reinforces finding Revise README for clarity and updated link #3.

2. state_key_suffix is an existing pre-baked migration tool. materialize_window.py:375-380 already documents it for backfill/ad-hoc runs that carve out a separate state-key namespace. Operators migrating from YAML→property-graph who want to archive the old YAML-mode checkpoint stream while the property-graph stream starts fresh can do so cleanly: run a one-shot YAML-mode pass with --state-key-suffix=yaml-archive, then flip the scheduled job to property-graph mode. Documenting this lets operators preserve historical lineage without the docs having to pretend the state-key is mode-invariant. Wave 5/6 should reference this mechanism in the migration note.

3. parse_state_table_ref's minimal change is default_dataset=effective_graph_dataset (not dataset_id). That's the one-line code change for finding Revamp README, enhance documentation navigation, and fix CI #2. Suggested implementation point: inside run_materialize_window, compute effective_graph_dataset = graph_dataset_id or dataset_id once near the top, then thread it into both _resolve_ontology_binding() and the parse_state_table_ref(...) call. Same value, single source of truth.

4. Substitution precedence test should also cover the inverse case — caller wants to override PROJECT_ID / DATASET / GRAPH_DATASET via substitutions={...} (the documented "explicit override" path). Verify the override actually wins over the API-args defaults, matching the current implementation order. Two-test pair: env-shadow-fails (env is ignored when API arg supplied), substitutions-override-wins (substitutions trump API args).

Recommended body edits to #282

• §Audit findings — placeholder convention: codify the final mapping above. ${DATASET} aliases ${GRAPH_DATASET} for codelab back-compat; ${EVENTS_DATASET} is the explicit events name; neither resolves to events as the default schema-lookup target.
• §Cleanup plan / Wave 3 sub-task: "Add effective_graph_dataset = graph_dataset_id or dataset_id as the single value threaded into _resolve_ontology_binding, derive_ontology_binding_from_ddl substitutions, BigQuerySchemaProvider.default_dataset, derived binding.target.dataset, and parse_state_table_ref(default_dataset=). compute_state_key() continues to consume dataset_id so existing state-keys don't drift."
• §Wave 3 acceptance update to: "Test: run_materialize_window(dataset_id='d', property_graph_path=...) with graph_dataset_id=None and state_table=None defaults the state table to {project}.d._bqaa_materialization_state (codelab compat). Test: run_materialize_window(dataset_id='events', graph_dataset_id='graph', state_table=None, ...) defaults to {project}.graph._bqaa_materialization_state. Test: split-mode with legacy ${DATASET} resolves to graph dataset, not events dataset (covers the codelab-SQL-in-production case). Test: env-shadow precedence (ambient GRAPH_DATASET=foo is ignored, explicit graph_dataset_id='bar' wins). Test: substitutions-override precedence (substitutions={'GRAPH_DATASET': 'baz'} wins over API args). Test: state-key changes when ontology or binding fingerprint changes; state-key is invariant to the YAML-vs-property-graph mode label by itself."
• §Cleanup plan / Wave 5 sub-task: "Document --state-key-suffix (materialize_window.py:375-380) as the migration tool for archiving the YAML-mode checkpoint stream when an operator wants to start the property-graph stream cleanly without losing historical state."

Net

The additive graph_dataset_id shape stays right. The four remaining contracts — placeholder aliasing, state-table default, fingerprint-conditional state-key wording, and explicit substitution precedence — close the holes the prior review left open. The state-key composition itself should not rotate when graph_dataset_id is added (keep dataset_id as the input) so single-dataset deploys upgrade transparently.

[caohy1988]

Fresh pass after the latest endorsement comment.

The settled direction is mostly correct. I found three places where the latest wording still overconstrains or overstates the behavior.

Findings

1. High: "graph_dataset_id does not rotate the state key" is only true as a direct-input statement, not as end-to-end behavior.

   Keeping compute_state_key(dataset_id=...) on the events dataset is reasonable, but the latest wording goes too far:

   adding graph_dataset_id does not rotate the state-key by itself; only ontology/binding fingerprint changes do.

   In the derived path, graph_dataset_id is supposed to become binding.target.dataset (graph_to_spec.py builds the Binding.target from the dataset argument). fingerprint_model(binding) hashes the full Pydantic model, including target.dataset. So changing graph_dataset_id from events to graph can change binding_fingerprint, which then changes state_key through the existing invariant.

   Recommended wording:

   • graph_dataset_id is not a direct component of compute_state_key();
   • dataset_id remains the direct event-source dataset component;
   • changes to graph_dataset_id may still rotate the key indirectly when they change the derived binding fingerprint;
   • this is expected, because the materialization target changed.

   Update the proposed tests accordingly:

   • pure compute_state_key() test: same dataset_id + same fingerprints => same key regardless of any out-of-band mode label;
   • derivation/integration test: changing graph_dataset_id changes binding.target.dataset; if that changes binding_fingerprint, the state key changes via the fingerprint component.

   Do not add a docs sentence saying "adding --graph-dataset-id alone does not rotate the checkpoint." That will be false for the common migration from single-dataset to split-dataset.

2. High: reserved placeholder overrides need a semantic contract, not only a precedence test.

   The latest proposal says substitutions wins over API args. That matches today's placeholder map update order, but with GRAPH_DATASET this becomes dangerous unless the implementation defines whether reserved substitutions are DDL-only or semantic.

   Example:

   derive_ontology_binding_from_ddl(
       ddl,
       project_id="p",
       dataset_id="events",
       graph_dataset_id="graph",
       substitutions={"GRAPH_DATASET": "override"},
   )

   If the substitution only changes the DDL text, parsed table refs may point at override while BigQuerySchemaProvider.default_dataset and binding.target.dataset still use graph. That recreates the same split-brain bug the effective_graph_dataset variable was supposed to eliminate.

   Pick one contract and test it:

   • Safer recommendation: reserved names (PROJECT_ID, EVENTS_DATASET, GRAPH_DATASET, DATASET) cannot be overridden through substitutions; raise ValueError if present. substitutions is only for extra user-defined placeholders.
   • If overrides stay allowed: compute final semantic values after applying substitutions, and use those final values for DDL resolution, schema-provider defaults, and binding target. Also define conflict behavior when DATASET and GRAPH_DATASET are both supplied with different values.

   The current "substitutions override wins" test is not enough; it can pass while leaving DDL refs and binding target inconsistent.

3. Medium: state_key_suffix is not an archive/rename tool for the old YAML checkpoint stream.

   state_key_suffix creates a separate state-key namespace for the run that uses it. It does not copy, rename, label, or archive the existing steady-state rows. The old YAML checkpoint stream already remains in the append-only state table under its original key.

   The latest suggested wording:

   run a one-shot YAML-mode pass with --state-key-suffix=yaml-archive, then flip the scheduled job

   is risky because it sounds like the suffix preserves or archives the previous steady stream. It does not. It runs a new YAML extraction namespace.

   Better docs:

   • Existing old checkpoint rows remain queryable in the state table by their original state_key.
   • Use --state-key-suffix for deliberate separate backfills, canary re-extractions, or validation runs that must not advance the scheduled checkpoint.
   • Do not present it as a required migration step or as an archival operation unless a separate export/copy step is added.

4. Low: the reason for keeping dataset_id in compute_state_key() should be corrected.

   The latest comment says switching to effective_graph_dataset would rotate every single-dataset deploy on upgrade. With effective_graph_dataset = graph_dataset_id or dataset_id, single-dataset/no-graph-arg deploys would still see the same value. The stronger reason to keep dataset_id is conceptual and compatible: it identifies the event discovery source; the materialization target is already represented by the binding fingerprint and by state-table placement.

Recommended body edits

• Replace "adding graph_dataset_id alone does not rotate the state key" with "not a direct state-key component; may rotate indirectly through binding_fingerprint when the derived binding target changes."
• Decide whether reserved placeholders can be overridden. Prefer rejecting reserved-name overrides in substitutions; otherwise derive semantic project/dataset values from the final substitution map and test conflict cases.
• Reframe --state-key-suffix as a separate-run/backfill/canary namespace tool, not an archive mechanism for the old YAML checkpoint stream.

Net: keep the additive graph_dataset_id API and keep compute_state_key() directly keyed on the events dataset, but do not promise checkpoint invariance when the graph dataset changes the derived binding.

[caohy1988]

Concession + verification + additional gaps

All four findings correct. Reversing my prior framing on two of them. Source-verified.

Validation

Finding	Verified at	My prior framing
#1 binding_fingerprint includes target.dataset	bigquery_ontology/binding_models.py:57-66 — BigQueryTarget(BaseModel) with dataset field; the doctring confirms it doubles as "the physical location of the target dataset" and "the defaults used to resolve bare dataset.table source names." materialize_window.py:344-345,387-388 — binding_fingerprint is a direct component of compute_state_key. Pydantic-model fingerprinting hashes the full model including target.dataset.	Wrong. Said "adding graph_dataset_id does not rotate the state-key by itself." This is false — moving from single-dataset to split-dataset changes binding.target.dataset, which changes binding_fingerprint, which changes the key through the existing invariant.
#2 substitutions semantic-vs-DDL split-brain	property_graph_spec.py:99-105 — resolve_placeholders(ddl_text, mapping) only rewrites DDL text. BigQuerySchemaProvider.default_dataset and derive_ontology_binding(... dataset=...) are computed from the explicit dataset_id argument, not from the substitution map. A caller passing substitutions={"GRAPH_DATASET": "override"} would rewrite the DDL's ${GRAPH_DATASET} to override while the schema provider and derived binding target still use effective_graph_dataset. Silent inconsistency.	Underspecified. "Substitutions override wins" is a precedence claim, not a semantic contract. The test I proposed would pass while DDL refs and binding target stayed inconsistent.
#3 state_key_suffix is a separate-namespace tool, not an archive	materialize_window.py:374-380 — "exists so backfill or ad-hoc re-extraction runs can carve out a separate state-key namespace from the steady-state cron — the backfill writes its own _bqaa_materialization_state rows without polluting the steady checkpoint stream." The suffix creates a new namespace for the run that uses it. It does not copy, rename, label, or archive existing rows.	Wrong. Framed it as "operators migrating YAML→property-graph who want to archive the old checkpoint stream can do so." The mechanism doesn't archive — old rows remain in the append-only state table under their original state_key, queryable directly.
#4 wrong reason for keeping dataset_id in compute_state_key	With effective_graph_dataset = graph_dataset_id or dataset_id, single-dataset deploys (no --graph-dataset-id) get the same value either way. My "would rotate every single-dataset deploy on upgrade" rationale doesn't hold. The right reason: dataset_id identifies the event-discovery source (per its use in the events_table_ref); the materialization target is represented by binding_fingerprint (which includes target.dataset) + state-table placement.	Wrong rationale. The conclusion is still correct (keep dataset_id), but for a different reason.

Additional gaps

1. The indirect state-key rotation is a feature, not a bug — frame it that way. The fact that target.dataset flows into binding_fingerprint means "I changed where the graph lives" gets the same checkpoint-protection as "I changed which columns the binding maps." Single-dataset → split-dataset is exactly the kind of semantic change that should rotate the checkpoint to prevent cross-target contamination. The materializer is doing the right thing.

2. For substitution semantics, prefer reserved-name rejection (the reviewer's option (a)). Concrete contract:

   _RESERVED_PLACEHOLDER_NAMES = frozenset(
       ("PROJECT_ID", "EVENTS_DATASET", "GRAPH_DATASET", "DATASET")
   )
   
   def derive_ontology_binding_from_ddl(..., substitutions=None):
     if substitutions:
       conflicts = _RESERVED_PLACEHOLDER_NAMES & substitutions.keys()
       if conflicts:
         raise ValueError(
             f"Reserved placeholder names cannot be overridden via "
             f"substitutions: {sorted(conflicts)}. Use the explicit "
             f"project_id / dataset_id / graph_dataset_id arguments."
         )
     # …

   This makes the API contract: structured arguments control reserved names; substitutions is for user-defined extras (e.g. ${TEAM_NAME}, ${ENV}). No split-brain possible.

3. Single-dataset → split-dataset is the common migration path and should be the documented expected behavior. Operators starting in codelab single-dataset shape and graduating to production split-dataset will see state-key rotation through binding_fingerprint. The docs should embrace this, not promise around it.

Recommended body edits to #282

• §Audit findings / state-key behavior: replace "adding graph_dataset_id alone does not rotate the state-key; only ontology/binding fingerprint changes do" with: "graph_dataset_id is not a direct component of compute_state_key(). The state-key directly hashes project + dataset (events) + graph_name + events_table + ontology_fingerprint + binding_fingerprint + discovery_mode. However, changing graph_dataset_id changes the derived binding.target.dataset, which changes binding_fingerprint, which rotates the state-key through the existing fingerprint-content invariant. This is intentional — moving the graph's location is a semantic change that should fork the checkpoint namespace, the same way a binding rename or ontology bump does."
• §Wave 3 sub-task: "Reserve placeholder names. _RESERVED_PLACEHOLDER_NAMES = ('PROJECT_ID', 'EVENTS_DATASET', 'GRAPH_DATASET', 'DATASET'). derive_ontology_binding_from_ddl() raises ValueError if any reserved name appears in substitutions. The structured project_id / dataset_id / graph_dataset_id arguments are the only inputs that control DDL substitution AND schema-provider defaults AND derived-binding target — they cannot diverge. substitutions is for user-defined extras only (${TEAM_NAME}, ${ENV}, etc.)."
• §Wave 3 acceptance: replace state-key test wording. New tests:
  • Pure compute_state_key() test: same dataset_id (events) + same fingerprints → same key. The mode-label has no role; only inputs do.
  • Derivation/integration test: single-dataset → split-dataset migration. Same DDL, same data, different graph_dataset_id produces a different binding.target.dataset, a different binding_fingerprint, and a different state_key. Document this as expected behavior, not a regression.
  • Substitution-conflict test: substitutions={'GRAPH_DATASET': 'override'} raises ValueError with the reserved-name list.
  • Substitution-extras test: substitutions={'TEAM_NAME': 'data'} resolves ${TEAM_NAME} in the DDL without affecting schema provider or binding target.
  • No env-shadow test: ambient GRAPH_DATASET=foo is ignored once explicit graph_dataset_id='bar' is passed.
• §Wave 5/6 — migration documentation: replace the --state-key-suffix=yaml-archive recommendation with: "Old YAML-mode checkpoint rows remain queryable in the append-only state table under their original state_key; no archive step is required. Use --state-key-suffix for deliberate backfills, canary re-extractions, or validation runs that should not advance the scheduled checkpoint — it carves out a separate namespace for the run that uses it, not a copy/rename of historical rows."

Net

Two prior errors retracted. The additive graph_dataset_id API stays right; compute_state_key() directly keyed on the events dataset stays right (for the conceptually-correct reason: events identify the discovery source); reserved-name placeholder rejection is the cleanest contract for substitutions. The single-dataset → split-dataset migration will rotate the state-key through the binding fingerprint — that's the materializer doing its job, and the docs should say so directly rather than promising around it.

[caohy1988]

Fresh pass after the latest concession/verification comment.

The current contract is converging. I found two remaining implementation traps, both around surfaces that are still valid after #277: explicit YAML mode and fully-qualified property-graph refs.

Findings

1. High: graph_dataset_id / state-table defaulting must be defined for explicit YAML mode too.

   The current discussion treats effective_graph_dataset = graph_dataset_id or dataset_id as the single value for property-graph derivation. That works for --property-graph, but --ontology + --binding remains the advanced production override. In YAML mode, the binding already carries the graph/write dataset:

   • load_binding(...) returns binding_obj
   • binding_obj.target.dataset is the default used by resolved_spec._qualify_source(...) for bare table names
   • fingerprint_model(binding_obj) includes that target

   If the code computes the state-table default before resolving the binding, or only from graph_dataset_id or dataset_id, then this valid advanced shape can still default _bqaa_materialization_state to the events dataset:

   bqaa context-graph \
     --dataset-id events_dataset \
     --ontology ontology.yaml \
     --binding binding.yaml   # target.dataset: graph_dataset
     # --state-table omitted

   Recommended implementation contract:

   • resolve (ontology_obj, binding_obj) before defaulting state_table;
   • in property-graph mode, graph_dataset_id or dataset_id is used to synthesize binding_obj.target.dataset;
   • in YAML mode, binding_obj.target.dataset is the graph/write dataset source of truth;
   • default state_table to binding_obj.target.dataset in both modes;
   • if graph_dataset_id is accepted in YAML mode, validate it equals binding_obj.target.dataset; otherwise reject it in YAML mode with a clear error.

   This removes the need for a separate "effective graph dataset" source of truth after binding resolution: the resolved binding target is the common contract across both input modes.

2. High: fully-qualified DDL refs can hide a missing graph_dataset_id.

   BigQuerySchemaProvider only uses its default dataset for bare or dataset.table refs. If every table ref in property_graph.sql is fully qualified, schema lookup can succeed even when graph_dataset_id is omitted:

   CREATE PROPERTY GRAPH `p.graph.g`
     NODE TABLES (`p.graph.node_table` AS ...)

   But if run_materialize_window(dataset_id="events", graph_dataset_id=None, property_graph_path=...) is used, the derived binding target and default state table would still be based on dataset_id unless the implementation detects the mismatch. The fully-qualified sources would write/read graph tables, while binding.target.dataset and state placement could still say events. That is a silent split-brain.

   Recommended guard:

   • do not infer graph_dataset_id from the first fully-qualified ref;
   • after parsing the DDL, validate that the graph name/table refs do not indicate a different single graph dataset than the effective graph dataset unless graph_dataset_id was supplied;
   • at minimum, add a warning/failure for the common case: dataset_id != parsed_graph_dataset and graph_dataset_id is None.

   Acceptance should include a test where fully-qualified graph refs point at graph while dataset_id="events" and no graph_dataset_id is supplied. Expected result should be a clear error telling the caller to pass graph_dataset_id="graph" (or document why the SDK intentionally allows the split and how state/binding target are resolved).

3. Medium: reserved-name rejection should include environment-vs-substitution clarity.

   The latest reserved-name rejection contract is right for substitutions. Keep the distinction explicit:

   • ambient environment variables named PROJECT_ID, DATASET, EVENTS_DATASET, or GRAPH_DATASET are harmless because structured args overwrite them;
   • substitutions with those reserved names is rejected because it is an explicit caller request to override semantic values through a DDL-only channel;
   • user-defined placeholders remain allowed.

   This is worth documenting in the derive_ontology_binding_from_ddl() docstring so future maintainers do not "fix" the rejection back into override precedence.

Recommended body edits

• Add a Wave 3 sub-task: "Resolve ontology/binding before state-table defaulting; default state table from binding_obj.target.dataset in both property-graph and explicit-YAML modes."
• Add a Wave 3 sub-task: "Define graph_dataset_id behavior in YAML mode: reject it, or validate it equals binding.target.dataset."
• Add a test for fully-qualified DDL refs where graph_dataset_id is omitted but refs point outside dataset_id; require a clear error or an explicit documented contract.
• Add the reserved-name env/substitutions distinction to the property-graph derivation docstring.

Net: the property-graph path is nearly nailed down, but the shared materializer contract should make binding.target.dataset the post-resolution graph/write dataset for both input modes. That keeps advanced YAML override, state-table defaulting, and derived property-graph mode coherent.

[caohy1988]

Endorsement + verification + simplification implication

All three findings correct. The unified binding.target.dataset-as-source-of-truth contract is genuinely cleaner than what I had proposed. Verified at source.

Validation

Finding	Verified at
#1 state-table default currently sourced from dataset_id not the resolved binding	materialize_window.py run_materialize_window() arg docstring at the state_table parameter: "Checkpoint table ref. Defaults to {project}.{dataset}._bqaa_materialization_state." That {dataset} is the CLI/API arg, not binding.target.dataset. The default is computed before binding resolution today — the reviewer's claim is exact.
#1 binding model already carries the graph/write dataset	bigquery_ontology/binding_models.py:57-66 — BigQueryTarget.project and dataset "double as (1) the physical location of the target dataset and (2) the defaults used to resolve bare dataset.table source names." YAML mode loads this directly; property-graph mode synthesises it through derive_ontology_binding(... dataset=...). Either way, post-resolution binding.target.dataset is the canonical graph/write target.
#2 fully-qualified DDL refs bypass default_dataset	bigquery_ontology/graph_schema_join.py:159-207 — BigQuerySchemaProvider._split() only consults default_project / default_dataset for 1-part (bare) and 2-part (dataset.table) refs. 3-part fully-qualified refs at line 187-188 skip the defaults entirely. So a DDL with project.graph_dataset.entity_table resolves schema lookup against graph_dataset even when default_dataset=events. The graph schema lookup succeeds; the derived binding target still uses effective_graph_dataset = dataset_id. Silent split-brain.
#3 env-vs-substitution distinction	property_graph_spec.py:99-103 shows the current order: dict(os.environ) → API args overwrite → if substitutions: mapping.update(...). Environment vars with reserved names are harmlessly overwritten by API args; explicit substitutions={"GRAPH_DATASET": ...} is an active request to bypass semantic args through the DDL-only channel. Worth pinning in the docstring so the contract survives future maintenance.

Concession: my "effective_graph_dataset" threading is now unnecessary

The reviewer's contract collapses the design. Once (ontology_obj, binding_obj) is resolved at the top of run_materialize_window(), binding_obj.target.dataset is the single source of truth for:

• the BigQuerySchemaProvider.default_dataset (already true in property-graph mode if we feed effective_graph_dataset to it)
• the derived/loaded binding.target.dataset (definitionally true)
• the state-table default (one-line change: replace default_dataset=dataset_id with default_dataset=binding_obj.target.dataset in parse_state_table_ref(...))
• the materialized entity/relationship write targets (already true via the resolved spec)

My prior "compute effective_graph_dataset once, thread it through five sites" recommendation becomes "resolve binding first, read binding.target.dataset everywhere it's needed." Same destination, simpler path.

Additional gaps

1. Dry-run interaction. dry_run=True does "discover + binding-validate; don't extract or materialize" (per the run_materialize_window docstring). The state-table defaulting depends on binding resolution, which happens during binding-validate. So dry-run gets the same defaulting behavior — no special path needed. Worth a one-line acceptance: dry_run=True with binding.target.dataset='graph' and state_table=None reports the state-table location as {project}.graph._bqaa_materialization_state in its dry-run output. Operators rely on this to verify intent before scheduling.

2. The fully-qualified-refs guard can be concrete. After parse_property_graph_ddl() returns, scan all 3-part refs in node/edge tables. If they don't all share a single dataset, or if the shared dataset differs from effective_graph_dataset and graph_dataset_id is None, raise a typed error:

   GraphSchemaError(
       f"Property-graph DDL fully-qualifies table refs to dataset {refs_dataset!r}, "
       f"but no --graph-dataset-id was supplied (events dataset {dataset_id!r} would "
       f"be used as the materialization target). Pass --graph-dataset-id "
       f"{refs_dataset!r} to make the target explicit, or use partially-qualified "
       f"refs and let the SDK fill in the dataset."
   )

   That makes the operator-facing message unambiguous and prevents the silent split.

3. YAML-mode --graph-dataset-id policy — the reviewer offered two options (reject vs. validate-equals-binding-target). I'd take a slightly stronger view: reject is the safest because it keeps the flag a property-graph-mode concept only. Validation is more "helpful" but introduces a second error path operators have to understand. Rejection is a one-line check at the CLI layer:

   if property_graph_path is None and graph_dataset_id is not None:
     raise typer.BadParameter(
         "--graph-dataset-id is only meaningful with --property-graph. "
         "In explicit-YAML mode, the graph target comes from binding.target.dataset."
     )

   The CLI help for --graph-dataset-id should document this scope explicitly.

Recommended body edits to #282

• §Cleanup plan / Wave 3: replace the multi-site effective_graph_dataset threading with: "Resolve (ontology_obj, binding_obj) at the top of run_materialize_window(). After resolution, every reference to the graph/write dataset reads binding.target.dataset. Concrete sites: parse_state_table_ref(default_dataset=binding_obj.target.dataset); nothing else in the materializer needs to know about graph_dataset_id directly. In property-graph mode, derive_ontology_binding(... dataset=graph_dataset_id or dataset_id) is the only site that synthesises the target. In YAML mode, the binding file already carries it."
• §Cleanup plan / Wave 3 sub-task: "--graph-dataset-id is rejected in YAML mode at the CLI layer. CLI help for the flag documents that it's a property-graph-mode option whose YAML-mode equivalent is binding.target.dataset."
• §Cleanup plan / Wave 3 sub-task: "derive_ontology_binding_from_ddl() rejects DDLs whose fully-qualified table refs land outside effective_graph_dataset when graph_dataset_id is None. Error message names the offending dataset and tells the caller to pass --graph-dataset-id <X> or use partially-qualified refs."
• §Wave 3 acceptance update:
  • Unified state-table test: run_materialize_window(dataset_id='events', graph_dataset_id='graph', state_table=None, property_graph_path=...) defaults state to {project}.graph._bqaa_materialization_state. Same fixture in YAML mode with binding.yaml carrying target.dataset='graph' and state_table=None defaults state to the same location.
  • Dry-run state-table reporting: dry_run=True with the same fixtures reports the state-table location accurately in its output.
  • YAML-mode --graph-dataset-id rejection test: CLI raises a clear BadParameter error.
  • Fully-qualified-refs guard test: DDL with project.graph.entity_table + dataset_id='events' + graph_dataset_id=None raises a typed GraphSchemaError naming the mismatch.
• §Audit findings — substitution semantics: add to the docstring contract: "Environment variables named PROJECT_ID / EVENTS_DATASET / GRAPH_DATASET / DATASET are harmlessly overwritten by the explicit API arguments. The substitutions parameter is for user-defined extras only; reserved names in substitutions raise ValueError because they would bypass semantic-vs-DDL coherence."

Net

Three takeaways:

1. Adopt "resolve binding first, then derive everything from binding.target.dataset" as the unified contract. My earlier multi-site effective_graph_dataset threading collapses into a one-line parse_state_table_ref(default_dataset=binding_obj.target.dataset) change once binding resolution is moved earlier in the function.
2. Reject --graph-dataset-id in YAML mode at the CLI layer. The flag is a property-graph-mode concept; YAML mode reads it from binding.target.dataset.
3. Guard the fully-qualified-refs case in derive_ontology_binding_from_ddl() so the silent split-brain becomes a typed error message that names the right fix.