#223: Databricks prune compiler emits valid Databricks SQL from DATABRICKS_DIALECT - #256
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDatabricks prune-compiler docs were updated, the import guard now forbids Databricks imports, compiler snapshots and fixtures were added, and an ungated ChangesDatabricks prune compiler validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
…rune/ import-guard
…s + ungated snapshot tests
…e-guard + docs
…terns + marker note Document the 3rd dialect cert in prune-engine.md (verify-don't-re-derive a skeleton dialect; ungated parse-guard DEC-002; unquoted Spark sample CTE) and warehouse-adapters.md (DATABRICKS_DIALECT offline-certified by #223, live deferred to #226). Clarify the databricks pytest marker now gates only the live cert — the offline sqlglot parse-guard is ungated.
Implementation complete ✅All 6 epic beads (
Key outcome: no Final validation: |
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 `@tests/prune/test_compiler_databricks.py`:
- Around line 77-80: The _fixture_id helper currently uses path.parent.name,
which collapses both fixture trees to the same parametrization ID. Update
_fixture_id in test_compiler_databricks to build the ID from the path relative
to _FIXTURES_ROOT so the anomaly/databricks branch is included and top-level
fixtures remain distinguishable. Keep the returned ID readable, but ensure it
uniquely reflects the full fixture location instead of only the immediate parent
directory.
🪄 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: 30616643-fef2-4de1-96fd-bb6714cd106b
📒 Files selected for processing (41)
.claude/rules/prune-engine.md.claude/rules/warehouse-adapters.mddocs/prune-ops.mdplans/super/223-databricks-prune-compiler.mdpyproject.tomlsrc/signalforge/warehouse/models.pytests/fixtures/prune/compiled_sql/anomaly/databricks/mad_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/mad_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/mad_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/mad_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/min_max_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/min_max_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/min_max_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/min_max_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/percentile_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/percentile_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/percentile_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/percentile_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/zscore_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/zscore_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/zscore_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/databricks/zscore_none_violation.sqltests/fixtures/prune/compiled_sql/databricks/accepted_values.sqltests/fixtures/prune/compiled_sql/databricks/accepted_values_sample.sqltests/fixtures/prune/compiled_sql/databricks/custom_sql.sqltests/fixtures/prune/compiled_sql/databricks/custom_sql_fullscan.sqltests/fixtures/prune/compiled_sql/databricks/custom_sql_sample.sqltests/fixtures/prune/compiled_sql/databricks/not_null.sqltests/fixtures/prune/compiled_sql/databricks/not_null_sample.sqltests/fixtures/prune/compiled_sql/databricks/relationships.sqltests/fixtures/prune/compiled_sql/databricks/relationships_sample.sqltests/fixtures/prune/compiled_sql/databricks/row_count_between.sqltests/fixtures/prune/compiled_sql/databricks/row_count_between_where.sqltests/fixtures/prune/compiled_sql/databricks/unique.sqltests/fixtures/prune/compiled_sql/databricks/unique_combination_pair.sqltests/fixtures/prune/compiled_sql/databricks/unique_combination_three_columns.sqltests/fixtures/prune/compiled_sql/databricks/unique_combination_with_where.sqltests/fixtures/prune/compiled_sql/databricks/unique_sample.sqltests/prune/test_compiler.pytests/prune/test_compiler_databricks.pytests/prune/test_compiler_import_guard.py
… parametrize id CodeRabbit: path.parent.name is 'databricks' for both the top-level and the anomaly fixture dirs, so the parametrize id couldn't tell them apart. Key the id off the path relative to the fixtures root instead — anomaly fixtures now report as 'anomaly/databricks/<name>'.
PR Review SummaryFixed (1 item)
False Positives (0 items)None. All CI green (lint-test 3.11/3.12/3.13, CodeRabbit, docs-build); pyright 0 errors; |
Summary
Super plan for #223 — make
_compile_testemit valid Databricks SQL purely fromDATABRICKS_DIALECT, proven by byte-exact fixtures + asqlglotdatabricks-dialect parse-guard.Phase: detailing (awaiting approval)
Stories: 4 implementation + Quality Gate + Patterns & Memory
Decisions: 5 captured (DEC-001…005)
Key discovery finding
A de-risking prototype compiled all 8 primitives × {full, sample} scope (incl. the anomaly two-query split across mad/zscore/percentile + dow) with
DATABRICKS_DIALECTand parsed every statement through sqlglot'sdatabricksdialect: 26/26 OK, 0 failures. The #221 dialect + dialect-driven compiler are already correct — this ticket is fixtures + tests + one import-guard line + docs.DATABRICKS_DIALECTchange needed (DEC-001).sampleCTE alias is valid Spark — no reserved-word collision (unlike Snowflake).Plan document
See
plans/super/223-databricks-prune-compiler.md.Next steps
Summary by CodeRabbit
sampleunquoted), and verification guidance.