#225: Databricks estimate_query_bytes — degrade + EXPLAIN COST (plan) - #259
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDatabricks ChangesDatabricks EXPLAIN COST Estimation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…-001)
Add the pure module-level _parse_explain_cost_bytes(cell) -> int parser to the
Databricks adapter, mirroring the Snowflake _parse_explain_json_bytes analogue
but parsing Spark/Databricks EXPLAIN COST plan TEXT (not JSON).
- _SPARK_DEFAULT_SIZE_SENTINEL_BYTES = 8 * 1024**6 (= 2**63, the Spark
defaultSizeInBytes no-stats sentinel, printed as 8.0 EiB).
- 1024-based unit table {B, KiB, MiB, GiB, TiB, PiB, EiB} + a regex that
extracts every Statistics(sizeInBytes=<num> <unit>) (int/decimal/scientific).
- Returns the MAX across all plan nodes (DEC-003 — the leaf scan is the
bytes-scanned cost proxy; the root output size understates it).
- Raises the existing EstimateUnavailableError (DEC-006, no new error class) on:
no matches (DEC-005), max >= sentinel naming ANALYZE TABLE (DEC-004), a
non-str cell, or a negative/non-finite parsed value.
Pure: no connection, no warehouse call, no logging.
New table-driven test file tests/warehouse/test_databricks_estimate.py covers
unit conversions, decimal/scientific values, max-across-nodes selection, a
realistic multi-line plan, the 8.0 EiB sentinel, no-sizeInBytes, non-str cells,
and the non-finite guard — all via synthetic inline plan-text (US-005 owns the
maintainer-captured fixture).
…ute_scalar Add DatabricksAdapter._execute_scalar (no-TableRef sibling of _execute, routing SDK errors through map_databricks_exception, closing the cursor in finally) and override estimate_query_bytes to run EXPLAIN COST <validated-sql> and parse the max Spark CBO sizeInBytes via _parse_explain_cost_bytes (validate_test_sql first; empty result -> EstimateUnavailableError, never a fabricated 0). Updates the module + class docstrings; drops the now-obsolete inherits-typed-degrade stub test for estimate (run_stats_query still degrades). #225 DEC-002/008/009/012.
…query_bytes via EXPLAIN COST
…ade pins Add the _ESTIMATE_SOURCE_LABELS entry 'DatabricksAdapter' -> 'Databricks EXPLAIN COST' (DEC-011 of #225) so the estimate renderer labels the warehouse cost source correctly. Pin two engine-level paths (US-003), mirroring the Snowflake engine tests: a real EXPLAIN COST plan (12.0 MiB leaf scan) -> real warehouse_total_bytes + adapter-derived source label; the 8.0 EiB no-stats sentinel plan -> warehouse_unavailable_reason starting with 'EstimateUnavailableError:' (DEC-012), engine never raises.
#225 US-005 (DEC-010, DEC-007): pin the pure _parse_explain_cost_bytes parser against committed file fixtures shaped like real Databricks EXPLAIN COST output. - tests/fixtures/warehouse/databricks/explain_cost_sample.txt — Optimized Logical Plan + Physical Plan over a parquet Relation leaf; the leaf scan carries Statistics(sizeInBytes=128.0 MiB) so the MAX-across-nodes parser returns int(128.0 * 1024**2) deterministically. - tests/fixtures/warehouse/databricks/explain_cost_no_stats.txt — the Statistics(sizeInBytes=8.0 EiB) no-CBO-statistics sentinel shape. - tests/fixtures/warehouse/databricks/README.md — maintainer capture/regen command (PAT + databricks-sql-connector one-liner) + the TO-BE-REPLACED placeholder note; live end-to-end validity deferred to #226. - Appended a fixture-pin section to tests/warehouse/test_databricks_estimate.py (existing US-001/US-002 tests untouched): sample fixture parses to the known leaf-scan bytes; no-stats fixture raises EstimateUnavailableError naming ANALYZE TABLE.
…en note + parser pin
…grade pins Add two CLI-level tests mirroring the Snowflake estimate pins: a happy path (DatabricksAdapter wired to a FakeDatabricksConnection returning an EXPLAIN COST plan with a known MAX leaf sizeInBytes -> exit 0, stdout shows the rendered estimate labelled 'Databricks EXPLAIN COST') and a no-stats graceful degrade (8.0 EiB sentinel plan -> EstimateUnavailableError per #225 DEC-004 -> exit 0, stdout shows '<unavailable: EstimateUnavailableError>', no traceback floor).
…COST estimate US-006 (#225 DEC-013). Document that DatabricksAdapter.estimate_query_bytes now overrides the ABC degrade via EXPLAIN COST (parse the max Spark CBO Statistics(sizeInBytes=...) across plan nodes), degrading to the reused EstimateUnavailableError on the 8.0 EiB / Long.MaxValue no-stats sentinel or an unparseable plan -- never reports the ~9-exabyte figure, never fabricates 0. - docs/warehouse-adapter-ops.md: dispatch summary (only run_stats_query still degrades); Databricks adapter section estimate note + #226 ledger; Query-bytes estimation section gains the Databricks override mechanism (text-not-JSON crux), the 8.0 EiB sentinel degrade, and the planner-estimate / ANALYZE TABLE freshness caveat mirroring Snowflake. - .claude/rules/warehouse-adapters.md: estimate_query_bytes graduate-degrade recipe is now a 3-instance precedent (BQ dry_run / Snowflake #130 EXPLAIN USING JSON / Databricks #225 EXPLAIN COST); #224 parenthetical + #226 live-cert ledger updated; Reference pointer to plans/super/225. #225 ships shape-certified (synthetic + maintainer-captured fixture); live EXPLAIN COST validity is #226. Reused EstimateUnavailableError (no new class). CLAUDE.md has no Databricks-estimate claim -- left untouched.
…atabricks EXPLAIN COST
- Add _execute_scalar passthrough-arm + dict-cursor coverage tests (uncovered patch lines flagged by review; mirror the Snowflake precedent). - Add TiB + PiB unit-conversion tests (regex/power-table arms unexercised). - Comment the unreachable value<0 defensive branch in _parse_explain_cost_bytes. - README + #226 ledger: no-stats 8.0 EiB sentinel arises for non-Delta/external tables/views (a Delta leaf carries real size via its txn log regardless of ANALYZE); note the defaultSizeInBytes==Long.MaxValue runtime assumption.
…eying (DEC-012) Capture the #225 lesson in warehouse-adapters.md: when a degrade-phase and its override-phase ship in ONE PR, key the surviving engine+CLI degrade pin on the LIVE runtime error (EstimateUnavailableError) the same PR keeps, never the transient EstimateNotSupportedError it deletes; remove (don't leave stale) the skeleton's now-false inherits-degrade test. Memory: signalforge-databricks-estimate-pattern.
There was a problem hiding this comment.
Pull request overview
Implements Databricks estimate_query_bytes using EXPLAIN COST, replacing the prior ABC “not supported” degrade with a real estimate derived from Spark plan text. This adds a pure plan-text parser, fixture pins, and end-to-end --estimate coverage so Databricks now mirrors the shipped Snowflake “EXPLAIN-based estimate + graceful degrade” flow.
Changes:
- Add Databricks
estimate_query_bytesoverride that runsEXPLAIN COST <sql>and parses the maxStatistics(sizeInBytes=...)across plan nodes, degrading via existingEstimateUnavailableErroron missing stats/sentinel. - Introduce a pure
_parse_explain_cost_bytesparser with extensive unit/scientific-notation/sentinel coverage, plus committed fixtures (placeholder shape + maintainer recapture workflow). - Wire CLI estimate-source labelling and add CLI/engine integration tests for both happy-path bytes and
<unavailable: EstimateUnavailableError>degrade.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_databricks_stub.py | Removes the obsolete “inherits EstimateNotSupportedError” stub test and updates narrative to point to the new estimate tests. |
| tests/warehouse/test_databricks_estimate.py | New parser + adapter tests covering unit conversion, max-node selection, sentinel degrade, cursor closing, and exception mapping. |
| tests/fixtures/warehouse/databricks/README.md | Documents fixture purpose and maintainer recapture steps for real Free Edition EXPLAIN COST output. |
| tests/fixtures/warehouse/databricks/explain_cost_sample.txt | Adds a sample EXPLAIN COST plan-text fixture with a pinned leaf sizeInBytes. |
| tests/fixtures/warehouse/databricks/explain_cost_no_stats.txt | Adds a no-stats/sentinel fixture exercising EstimateUnavailableError behavior. |
| tests/cli/test_generate_estimate.py | Adds Databricks generate --estimate tests for real estimate + clean degrade (exit 0, no traceback). |
| tests/cli/test_estimate_engine.py | Adds engine-level Databricks estimate tests including correct warehouse_estimate_source labeling and degrade behavior. |
| src/signalforge/warehouse/adapters/databricks.py | Implements _parse_explain_cost_bytes, _execute_scalar, and estimate_query_bytes via EXPLAIN COST. |
| src/signalforge/cli/_estimate.py | Adds "DatabricksAdapter": "Databricks EXPLAIN COST" label for rendered estimate source. |
| plans/super/225-databricks-estimate.md | Adds the super plan detailing decisions, acceptance, and testing strategy for #225. |
| docs/warehouse-adapter-ops.md | Documents Databricks estimation mechanism, sentinel degrade behavior, and planner-estimate caveats; updates live-cert ledger. |
| .claude/rules/warehouse-adapters.md | Updates adapter graduation guidance with Databricks as the third estimate-graduation instance and extends the #226 live-cert ledger. |
💡 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plans/super/225-databricks-estimate.md`:
- Around line 67-75: Markdownlint is flagging this plan doc because the fenced
block is unlabeled and some inline code spans have extra interior spaces. Update
the affected fenced sections in the doc to use a language tag like text, and
trim the spaces inside the inline code spans so they render cleanly. Apply the
same cleanup in all matching sections mentioned by the review, including the
ones associated with the repeated plan snippets.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa378a90-dd88-42a3-85ca-5580d72bccbc
📒 Files selected for processing (12)
.claude/rules/warehouse-adapters.mddocs/warehouse-adapter-ops.mdplans/super/225-databricks-estimate.mdsrc/signalforge/cli/_estimate.pysrc/signalforge/warehouse/adapters/databricks.pytests/cli/test_estimate_engine.pytests/cli/test_generate_estimate.pytests/fixtures/warehouse/databricks/README.mdtests/fixtures/warehouse/databricks/explain_cost_no_stats.txttests/fixtures/warehouse/databricks/explain_cost_sample.txttests/warehouse/test_databricks_estimate.pytests/warehouse/test_databricks_stub.py
CodeRabbit MD040/MD038: label the EXPLAIN COST example fence (```text) and trim the trailing space inside the three `EXPLAIN COST` inline code spans.
Owner
Author
PR Review SummaryFixed (1 item)
False Positives (0 items)None. All CodeRabbit findings were doc-only markdownlint nits in the internal plan document and have been fixed. No production-code or test findings were raised. CI is green (lint-test 3.11/3.12/3.13 + docs-build). |
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.
Summary
Super plan for #225 — Databricks
estimate_query_bytes(degrade-first, thenEXPLAIN COST). The direct twin of the shipped Snowflake #123 (degrade) + #130 (EXPLAIN), with the dependency (#224 sampling) already merged.Phase: detailing (awaiting approval)
Stories: 6 implementation stories + Quality Gate + Patterns & Memory
Decisions: 13 captured
Key decisions
EXPLAIN COSTestimate.Statistics(sizeInBytes=N <binary-unit>), return the max across nodes (DEC-003).8.0 EiB/Long.MaxValueno-stats sentinel →EstimateUnavailableError— never report a 9-exabyte cost (DEC-004).EstimateUnavailableError(feat: Snowflake estimate_query_bytes — EXPLAIN-based estimation (Phase 2 of #123) #130 shipped it) — no new error class, no exit-code/scan-7 churn (DEC-006).EstimateUnavailableError, not the transientEstimateNotSupportedError(DEC-012) — no intra-PR churn.Plan document
See
plans/super/225-databricks-estimate.md.Next steps
Summary by CodeRabbit
EXPLAIN COST, with reports showing the estimate source when available.EXPLAIN COSTfixtures.