21: Run AR-B1 cost-model probe + fix BQ adapter TableRef bug - #23
Merged
Conversation
Records the verified bytes-billed figure for US-003 in docs/prune-ops.md and bundles the adapter bug surfaced by the live run. - AR-B1 probe ran 2026-05-01 against bigquery-public-data.iowa_liquor_sales.sales (~30M rows, ~24 cols) for a 100k-row deterministic sample. BigQuery's pre-execution analyzer estimated 9,924,771,840 bytes (~9.92 GB) — ~99x the Phase-1 estimate and ~2x the probe's 5 GB sanity ceiling. The adapter's 100 MB cost cap (DEC-005 of #3) blocked execution before any bytes were billed; the figure is captured from the bytesBilledLimitExceeded error. - docs/prune-ops.md Cost model section: replaced the TBD placeholder with the verified figure and run date, and rewrote the surrounding paragraphs to call out that Q4=A is NOT adequate for v0.1 sample-mode on wide tables. Schema-only stays the v0.1 default. - Adapter bug (separate from US-003): BigQueryAdapter._get_table and the probe were passing our Pydantic TableRef directly into google.cloud.bigquery.Client.get_table, which only accepts str | TableReference | Table | TableListItem and explodes on TableRef.path access. Both call sites now pass qualified_name. FakeBigQueryClient._coerce_to_tableref learned to parse the dotted string, so the unit suite still binds the production path. The same shape was present in the integration tests at tests/warehouse/test_bigquery_integration.py — those would have failed identically against live BQ; they're now reachable as a side-effect. - Q4=C escalation tracked in #22 (temp-table-materialised sample for v0.2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR records the results of the AR-B1 BigQuery deterministic-sample cost probe in the ops documentation and fixes a BigQuery adapter bug where an internal TableRef model was passed directly into google.cloud.bigquery.Client.get_table(...) (which expects a string or SDK table reference types).
Changes:
- Update BigQuery
get_tablecall sites to passTableRef.qualified_name(string) rather than the internalTableRefobject. - Extend the BigQuery test fake to coerce dotted string table identifiers into
TableReffor unit tests. - Update
docs/prune-ops.mdwith the verified (analyzer-estimated) bytes figure from the AR-B1 probe run and guidance on Q4=A inadequacy for wide tables.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/warehouse/test_sample_cost_probe.py |
Fixes probe to call get_table with a supported identifier type (qualified_name). |
tests/warehouse/_fake.py |
Adds string-to-TableRef coercion to keep unit tests aligned with new call shape. |
src/signalforge/warehouse/adapters/bigquery.py |
Fixes adapter _get_table to pass qualified_name (string) to the BigQuery SDK. |
docs/prune-ops.md |
Records verified probe estimate (9.92 GB) and documents implications for v0.1 sample-mode cost model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prune-ops.md`:
- Around line 192-193: The docs refer to a per-call config named
`cost_limit_bytes` but the project/adapters use `maximum_bytes_billed`; update
the reference in the sentence (and any surrounding mentions in the same
paragraph) to `maximum_bytes_billed` so terminology matches the config/API, and
ensure any examples or flags use the `maximum_bytes_billed` symbol instead of
`cost_limit_bytes`.
- Around line 180-184: Update the follow-up note to reference the correct error
type: replace the mention of "InternalServerError" with
"bytesBilledLimitExceeded" so maintainers are directed to the correct exception
path; search for the string "InternalServerError" in the note and change it to
"bytesBilledLimitExceeded" and ensure the sentence about extracting the estimate
refers to the "bytesBilledLimitExceeded" message/exception.
In `@tests/warehouse/_fake.py`:
- Around line 167-169: The coercion for string refs in tests/warehouse/_fake.py
assumes three dotted parts and fails for two-part qualified names; update the
logic in the branch that handles isinstance(ref, str) so it splits ref on "."
and, if there are three parts, sets project,dataset,name, but if there are two
parts, sets project to None (or omits it) and assigns dataset,name accordingly
for TableRef(…), and raise a clear error for any other number of segments; refer
to TableRef and qualified_name to locate and adjust this conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1e1b481-d19c-4e5e-b70d-9873bf2ba2f9
📒 Files selected for processing (4)
docs/prune-ops.mdsrc/signalforge/warehouse/adapters/bigquery.pytests/warehouse/_fake.pytests/warehouse/test_sample_cost_probe.py
- _coerce_to_tableref accepts 2-part 'dataset.table' refs (Pydantic TableRef.qualified_name omits the project segment when project=None); raises a clear AssertionError for other shapes. (Copilot, CodeRabbit) - docs/prune-ops.md: lead with the BigQuery reason code bytesBilledLimitExceeded as the durable identifier; flag that the SDK exception class is unstable across versions (adapter expects BadRequest/400, live 2026-05-01 run with google-cloud-bigquery 3.41.0 raised InternalServerError/500). (Copilot, CodeRabbit) - docs/prune-ops.md: replace the wrong cost_limit_bytes reference with the actual profile-level maximum_bytes_billed field; point at warehouse-adapter-ops for context. (CodeRabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryFixed (3 items)
False Positives (0 items)None — all five comments were real issues (two duplicate pairs across Copilot + CodeRabbit on the same lines). |
Captures the lesson from the AR-B1 cost-probe failure: don't pass our Pydantic TableRef directly into vendor SDK methods that accept str | TableReference | Table | TableListItem. Always pass ref.qualified_name. Integration tests against the real SDK are the only thing that catches this; FakeBigQueryClient won't. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #21.
Summary
bigquery-public-data.iowa_liquor_sales.sales(~30M rows, ~24 cols) for a 100k-row deterministic sample. Verified figure: 9,924,771,840 bytes (≈9.92 GB) — ~99× the Phase-1 estimate and ~2× the probe's 5 GB sanity ceiling. BigQuery's pre-execution analyzer rejected the query because the adapter's 100 MBmaximum_bytes_billedcap (DEC-005 of BigQuery warehouse adapter with sampling + dialect helpers #3) is below the analyzer's estimate; the figure is captured from thebytesBilledLimitExceedederror rather thantotal_bytes_billedoff a completed job.docs/prune-ops.mdCost model section now records the verified figure + run date, and explicitly states Q4=A is not adequate for v0.1 sample-mode on wide tables. Schema-only stays the v0.1 default.Side-quest: BQ adapter
TableRef/get_tablebugThe probe's first failure was an
AttributeError: 'TableRef' object has no attribute 'path'from inside the BQ SDK. Root cause:BigQueryAdapter._get_table()and the probe both passed our PydanticTableRefstraight intogoogle.cloud.bigquery.Client.get_table(...), which only acceptsstr | TableReference | Table | TableListItem. Both call sites now passqualified_name(a string), andFakeBigQueryClient._coerce_to_tablereflearned to parse the dotted form so unit tests still bind the production path.Same call shape exists in
tests/warehouse/test_bigquery_integration.py(sample_rows,column_stats) — those would have failed identically against live BQ; they were never run live. The fix makes them reachable as a side-effect; live verification remains a separate concern (BQ-gated, opt-in viaSF_RUN_BQ=1).Test plan
pytest -q— 725 passed, 9 deselected.SF_RUN_BQ=1 pytest -m bigquery tests/warehouse/test_sample_cost_probe.py -v -s— surfaces the 9.92 GB figure viabytesBilledLimitExceeded. Probe currently fails (rather than xfails) on that branch; refining the probe to extract the estimate andxfailcleanly is noted inline indocs/prune-ops.mdas a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit