Lower governed definitions to Snowflake and Databricks - #273
Open
jat255 wants to merge 5 commits into
Open
Conversation
jat255
marked this pull request as draft
September 4, 2026 05:49
jat255
added
needs-manual-review
jat255
force-pushed
the
jat255/yfan-warehouse-emitters
branch
from
September 5, 2026 04:58
8fec6c7 to
9177fe4
Compare
|
Preview deployed to Connect ( Deployed from commit 81d3b26. |
|
Preview deployed to Connect ( Deployed from commit 81d3b26. |
jat255
force-pushed
the
jat255/yfan-warehouse-emitters
branch
from
September 6, 2026 02:07
9177fe4 to
8aac39d
Compare
Collaborator
Author
|
Reviewed 2026-09-05 21:24 MDT — manual review complete. Findings: 1 HIGH (Python had no coverage of the unsupported-construct → construction-failure path), 3 MEDIUM (silent single-target vs all-targets parity gap; shared fixture never exercised composition/marker substitution; mistyped fixture keys would pass while asserting nothing), 1 LOW (kata reference dressed as a GitHub issue link in tests/shared/README.md). All fixed in 73d7563 + c457148: error cases and a composition case now live in the shared fixture and run in both suites (the two hand-written R test blocks they duplicated are removed), both runners reject unknown expectation keys, the parity difference is documented in _compile.py, and the README no longer references the dev-local ledger. The parity gap itself is accepted as an implementation difference. Python: ruff/pyrefly clean, 696 passed; fixture verified to bite by perturbation. R runner changes are parse-checked but could not run locally (local R library has ellmer 0.4.2 < required 0.5.0) — CI is the verification there. roborev clean on both commits. |
jat255
removed
the
needs-manual-review
jat255
marked this pull request as ready for review
September 6, 2026 05:07
Stage 2 of the definition compiler (kata yfan), split out of f6hz once stage 1 landed. Ports pkg-r/R/definition-emit-sql.R, so a definition can be compiled for a warehouse rather than only for DuckDB. A source of either dialect now compiles instead of being refused at construction. These targets have no upstream authority. The pinned data-dict binary emits SQL(duckdb) and R targets only, so unlike every other part of the compiler there is nothing to conform against; kata g0ax is the upstream ask that would remove both emitters. The expectations therefore live in tests/shared/definition-warehouse-sql.json, hand-maintained, read by both suites, and the R side's hand-written copies are replaced by a runner over it. That is what the shared-fixture rule asks for and it is why this commit touches pkg-r at all. The two dialects share precedence and most syntax, so they are one writer parameterised by dialect, matching the R structure. Their differences are concentrated where lowering has to be target-aware: identifier quoting, struct access, datetime construction, guarded division and modulus, non-finite literals, and regular expressions. A construct a dialect cannot express raises inside the walk and is returned as an error on the translation rather than propagating, so one definition that cannot be lowered does not stop the others being emitted. The chosen target's error is what fails construction. Both runners were confirmed to bite by perturbing one expected string in the fixture and watching each suite fail.
…uoting Databricks quotes identifiers with backticks, and the composer only recognised double quotes, so a definition referencing a sibling kept its marker: `__commons_definition_reference_001__` reached the warehouse as a column that does not exist. R's composer takes the target for exactly this reason and I dropped the argument when porting it. Rather than test one dialect's quoting, the invariant is asserted over the whole corpus for every dialect commons lowers to: no compiled SQL may retain a marker. That is the class the bug belonged to, and it fails for Databricks when the quoting is reverted. Found by roborev job 326.
…ixture Review follow-ups: - Test the unsupported-construct path in Python: a standalone interval, a dynamic ROUND() scale, and a nanosecond datetime each fail construction for the dialect that cannot express them, mirroring the R suite's coverage of the same contract. - Pin composition in the shared fixture: survey::long postal references postal length, so its composed_code asserts the sibling is inlined with the target's identifier quoting, in both suites. The R runner recompiles the corpus with a mock source on the case's dialect, since composed SQL exists only for the source's own target. - Both runners reject unknown expectation keys, so a mistyped key can no longer pass while asserting nothing. - Document that Python lowers only the source's own target, where the R implementation emits every target and keeps them on the definition's translations. - Drop the kata reference from tests/shared/README.md; the ledger is developer-local and READMEs should not point at it.
Roborev flagged the Python refusal tests as a per-language copy of behavior both implementations must agree on. The unsupported constructs (a standalone interval, a dynamic ROUND() scale, a nanosecond datetime) now live in definition-warehouse-sql.json's new `unsupported` section, with inline dictionaries rather than corpus keys: `error_contains` pins the refusal and the accepting target pins `code`. Both suites run the section — Python by asserting construction fails for the refusing dialect, R by reading the error off the translation record and asserting a source on that dialect aborts — replacing the two hand-written R test blocks they duplicate. Perturbing an expected string fails each runner.
attach_compiled_definitions gained a required exposed argument in the stack below; these call sites predate it.
jat255
force-pushed
the
jat255/yfan-warehouse-emitters
branch
from
September 6, 2026 05:34
c457148 to
81d3b26
Compare
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
1 participant
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.
Stage 2 of the definition compiler (kata yfan), split out of f6hz once stage 1 landed. Stacked on #272. Ports
pkg-r/R/definition-emit-sql.R, so a source on either warehouse compiles its definitions instead of being refused at construction.These targets have no upstream authority
Every other part of the compiler is checked against the pinned
data-dictbinary. These emitters cannot be: the binary emitsSQL(duckdb)and three R targets, and nothing else. So there is no output to generate from and nothing external that says what correct looks like. kata g0ax is the upstream ask that would let one Rust implementation serve both languages and delete both emitters.The expectations instead live in
tests/shared/definition-warehouse-sql.json, hand-maintained, keyed to the same corpus dictionaries as the other definition fixtures, and read by both suites. Cases pincodeexactly where the whole string is worth reading, andcode_containsornotes_containwhere a rule is the point and the full rendering would bury it.Both runners were confirmed to bite by perturbing one expected string in the fixture and watching each suite fail.
The port
Snowflake and Databricks share precedence and most syntax, so they are one writer parameterised by dialect, matching the R structure. The differences are concentrated where lowering has to be target-aware: identifier quoting, struct access, datetime construction, guarded division and modulus, non-finite literals, and regular expressions.
A construct a dialect cannot express raises inside the walk and is returned as an error on the translation rather than propagating, so one definition that cannot be lowered does not stop the others. Only the chosen target's error fails construction.
Verification
578 Python tests, ruff and pyrefly clean. Full R suite 6588 passing, 0 failures.
R changes
The R package gains no new behaviour here.
R/is untouched. The change is totests/testthat/test-definition-compile.R, and it is a test-provenance change rather than a behavioral one.