Bind definitions to a source and wire both compiler phases - #272
Open
jat255 wants to merge 4 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/m2-f6hz-compile-and-wiring
branch
from
September 5, 2026 04:58
2e4237d to
6ef108a
Compare
Last of four PRs porting the definition compiler (kata f6hz, stage 1). Phase 1 runs when a dictionary is read, so an unusable definition fails at data_dictionary() before any source exists, which is where R reports it too. Phase 2 runs at data_source(), where a dialect is finally known, and fills the compiled_definitions the registry has been rendering since #261. Composition inlines a definition's sibling references into its SQL and merges their notes. Each reference is renamed to a unique marker before the SQL is emitted, rather than searching the emitted SQL for the definition's own name afterwards, which is what makes a collision impossible. The substitution is a scan rather than a string replace, so a string literal holding a marker is left alone. Grain is derived from the typed IR because the exported kind cannot answer it: a row expression can hold an aggregate child without becoming an aggregate. A metric whose chain mixes grain is refused, since one SQL expression cannot express the subquery rewrite it would need. Only DuckDB is lowered, so a source of any other dialect is refused at construction rather than emitted for wrongly. A dictionary with no definitions needs no emitter and is unaffected. End to end, the compiled SQL is executed against a real DuckDB source rather than only compared with a fixture, and every corpus definition is parsed by DuckDB. That parse check asserts the returned json_serialize_sql payload: it reports a parse failure in its result rather than raising, so the first version of the test passed unconditionally. export_spec() now accepts dictionary entries as models as well as raw mappings, because callers construct DataDictionary both ways and phase 1 runs for both.
build_registry() already caught this, but it runs at agent construction, by which time the compiled records have reached the dictionary's retrieval chunks and can describe a table the agent cannot query. R checks it at bind time for the same reason. Prose about an unexposed table is still fine. Only a definition emits SQL against a relation that has to be there. Found by roborev job 323.
jat255
force-pushed
the
jat255/m2-f6hz-compile-and-wiring
branch
from
September 6, 2026 02:07
6ef108a to
5b4a53e
Compare
Review follow-ups: - attach_compiled_definitions assigns nothing until every table compiles, so a refusal leaves the dictionary untouched and reusable for another source, and the exposed-table set is now required so the check cannot be skipped by omitting an argument. - Tests now pin that a string literal holding marker text and a column named like a marker are both left alone; the previous test only covered a definition's own name, which is never a substitution target. - Docstrings record where reading a dictionary and constructing a source can now fail, and the definition_exports comment names the actual value shape.
…ontract The IR walkers that rename references and find children stepped into lists of nodes and into mappings, but not into mappings inside lists — which is how a CASE stores its branches. A definition that referenced a sibling inside a CASE emitted the sibling's name as a quoted column that does not exist, and the corpus's own enterprise_revenue compiled to SQL no table can answer. A shared fixture is what should have caught this, so definitions.json gains a composed section: the composed DuckDB SQL and merged notes for every valid corpus definition, asserted by both suites. Composition is commons' own step, so the section is hand-maintained like mixed_grain and the generator preserves it. The R suite's hardcoded composed-SQL expectation is replaced by the fixture runner. Both implementations agree on all 42 cases.
Collaborator
Author
|
Reviewed 2026-09-05 21:20 MDT (final manual review pass). Findings from the review were addressed in 966cb21 and 0e56cab:
Checks on the pushed head: ruff clean, pyrefly 0 errors, 689 tests pass; the new R fixture runner passes (85 expectations). |
jat255
removed
the
needs-manual-review
|
Preview deployed to Connect ( Deployed from commit 0e56cab. |
|
Preview deployed to Connect ( Deployed from commit 0e56cab. |
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.
Last of four PRs porting the data-dict definition compiler (kata f6hz, stage 1). Stacked on #271. With this, a governed definition authored in data-dict's expression language reaches the agent as executable SQL.
Phase 1 runs when a dictionary is read, so an unusable definition fails at
data_dictionary()before any source exists. Phase 2 runs atdata_source(), where a dialect is finally known, and fills thecompiled_definitionsthe registry has been rendering since #261. That is the same split R uses, so both packages report a bad definition at the same constructor.Composition inlines a definition's sibling references and merges their notes. Each reference is renamed to a unique marker before the SQL is emitted, rather than searching the emitted SQL for the definition's own name afterwards, which is what makes a collision impossible. The substitution scans rather than string-replaces, so a string literal holding a marker is left alone.
Grain comes from the typed IR because the exported kind cannot answer it: a row expression can hold an aggregate child without becoming an aggregate. A metric whose chain mixes grain is refused, since one SQL expression cannot express the subquery rewrite it would need.
Only DuckDB is lowered, so a source of any other dialect is refused at construction rather than emitted for wrongly. Snowflake and Databricks are stage 2 of the issue.
Verification
End to end against a real DuckDB source: the compiled SQL is executed, not just compared with a fixture, and expanded
{{token}}queries return the right rows. Every corpus definition is also parsed by DuckDB.That parse check is worth a reviewer's eye.
json_serialize_sqlreports a parse failure in its returned payload rather than raising, so the first version of the test passed unconditionally. It now asserts the payload, and was confirmed to fail when the emitter leaves aCASEunterminated.export_spec()now accepts dictionary entries as models as well as raw mappings, because callers constructDataDictionaryboth ways and phase 1 runs for both. That was a regression caught by the existing registry tests.568 tests pass, ruff and pyrefly clean.
R changes
Nothing under
pkg-r/R/changes here. R runtime behavior is identical before and after, and the only R changes are in the test suite to use shared fixtures.