Type-check definition expressions and resolve their references - #270
Draft
jat255 wants to merge 9 commits into
Draft
Type-check definition expressions and resolve their references#270jat255 wants to merge 9 commits into
jat255 wants to merge 9 commits into
Conversation
Second of four PRs porting the definition compiler (kata f6hz). Turns a parsed expression into an export record: inferred type and kind, direct column and sibling-definition references, and the checks that refuse a definition before a conversation can reach it. Translations are absent by design; the DuckDB emitter is the next PR, and `translations` stays empty until it lands. Definitions resolve in dependency order rather than authored order, so a definition may be written before the sibling it uses, and a round that can resolve nothing is the cycle error. Regex validation is written against Rust's `regex` grammar rather than against R's guard list. R guards PCRE-only constructs because it falls back to PCRE; Python's `re` diverges from Rust somewhere else, so copying R's list would be wrong in both directions. The failure this prevents is compiling a pattern data-dict rejects. Verified against the installed data-dict binary, not only the fixture. The corpus tests were confirmed to bite by perturbing kind inference, reference deduplication, and the column-shadowing check. Removing the cycle guard hangs the resolver rather than failing a test, which is itself the evidence that the guard is what terminates it.
Compiling a pattern with Python's `re` to decide validity refused
dictionaries data-dict accepts. Verified against the binary: `\p{L}+`
exports as a filter emitting regexp_full_match("region", '\p{L}+'), and
Python's `re` cannot compile it at all. Rust-spelled named groups,
`(?<name>...)`, fail the same way.
Python's `re` is not a proxy for Rust's `regex` in either direction. It
accepts lookaround and backreferences Rust rejects, which the existing
guard catches, and it rejects Unicode classes and named groups Rust
accepts, which is what this fixes. Validity is now the guard alone.
Column selection is the one place commons must run the pattern itself
rather than pass it through, so that path still compiles, translating
Rust's named-group spelling first. When Python still cannot run it, the
error says commons cannot evaluate the selector rather than calling the
pattern invalid, and points at selecting by name.
That leaves a divergence from R, which reaches PCRE and can run these
patterns in a selector. It is rare, fails closed at construction with an
actionable message, and is recorded rather than engineered away.
Found by roborev job 313.
Two defects in the previous commit.
Dropping Python's compile check to stop refusing `\p{L}` also stopped
refusing patterns no engine can parse. `SIMILAR TO '('` exported cleanly
and would have reached the warehouse as broken SQL at conversation time.
data-dict rejects it with S21, verified against the binary. The parse
check is back, run against a probe in which the constructs Rust has and
Python lacks are neutralized, so it answers "does this parse" without
answering "does Python know this syntax". `\k<name>` joins the guard,
since Rust has no backreferences in any spelling.
The `(?<` to `(?P<` translation was a global substitution, so it also
rewrote the three literal characters inside a class: `[(?<]` became
`[(?P<]`, which selects a column named P. It is now a single left-to-right
scan that tracks character classes and copies escapes through.
Found by roborev job 315.
…ng Rust
Three rounds of review found three more ways a Python-based probe diverges
from Rust's regex: \z, malformed \p{...} forms slipping through
neutralization, and POSIX classes breaking the character-class scan. Each
fix produced the next divergence, because Python's re is not a proxy for
Rust's regex and cannot be made into one.
So this stops approximating. Patterns are validated, and column selectors
matched, with RE2 through the DuckDB already in commons' dependencies. RE2
and Rust's regex are both finite-automata engines: both reject lookaround
and backreferences, and both accept Unicode classes, \z and POSIX classes.
Every case raised across the three reviews now behaves correctly, and they
are pinned as two parametrized tables rather than as one-off tests.
This deletes the hand-maintained guard list and the character-class scanner
along with it, and removes the divergence from R that the first fix
introduced: a \p{L} column selector now works rather than being refused.
One known difference remains, and it fails closed: Rust spells a named
group (?<name>...) and RE2 wants (?P<name>...), so Rust's spelling is
refused, with a message that says so. Verified against the binary, which
accepts it. A capture name has no effect on a definition.
Found by roborev jobs 313, 315 and 316.
Both verified against the binary as accepted by data-dict: the named-group spelling (?<a>x), and extended mode (?x). Every other inline flag, i, s, m and U, agrees between the two engines. Pinned so the behaviour is deliberate and visible rather than a gap someone rediscovers. Both fail closed at construction with a message, and neither changes what a definition matches: a capture name is unused, and extended mode is only whitespace and comments.
The previous commit asserted there were exactly two, and (?R), Rust's CRLF-aware multiline flag, is a third. Verified against the binary: data-dict accepts it, RE2 refuses it. The defect was the exhaustiveness claim rather than the missing case, so the claim is gone and the test says so; the list records what has been found, and what all of them have in common is that they fail closed and none changes what a definition matches. The refusal message offered `(?P<name>...)` advice on every invalid pattern, including ones with no named group in them, which misdescribed the cause. It is now attached only when the pattern contains `(?<`, and a test asserts an unrelated refusal does not carry it. The parametrized `why` argument was unused, which implied a per-case message assertion that was not happening. Removed. Found by roborev job 318.
`(?<=x)` and `(?<!x)` share the `(?<` prefix with Rust's named-group spelling but are a different refusal: RE2 has no lookaround at all. The check now requires a name to follow, and the test covers both lookbehind forms alongside an unrelated malformed pattern. Found by roborev job 319.
`(?<)` and `(?<1>x)` are not valid Rust named groups, so the spelling advice did not apply to them either. The check now requires a letter or underscore, and the test covers both alongside the lookbehind forms. Found by roborev job 320.
jat255
marked this pull request as draft
September 4, 2026 05:49
jat255
added
needs-manual-review
…ction google-re2 is Google's own binding, maintained alongside RE2 itself, with prebuilt wheels for the whole supported matrix (cp310-cp314, macOS, Linux, Windows). It replaces the shared DuckDB connection, its lock, and the deferred import with direct compile and search calls, and it removes the read-time coupling to DuckDB that the PR description flagged. Its RE2 is also newer than DuckDB's vendored copy: Rust's named-group spelling `(?<name>...)` has compiled since RE2's 2023-07 release, so that divergence from data-dict disappears and the spelling hint goes with it. The remaining known differences are extended mode `(?x)` and CRLF-aware multiline `(?R)`, both still failing closed at construction. Verification: 483 tests pass, ruff and pyrefly clean on src and tests.
jat255
removed
the
needs-manual-review
Collaborator
Author
|
Approved after swapping to |
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.
Second of four PRs porting the data-dict definition compiler to Python (kata f6hz, stage 1). Stacked on #269. Turns a parsed expression into an export record: inferred type and kind, direct column and sibling-definition references, and the checks that refuse a bad definition at construction.
translationsstays empty by design. The DuckDB emitter is the next PR, so this PR assertskind,type,columnsanddefinitionsfor all 42 corpus definitions and leaves the SQL to #3.Verified against the installed
data-dictbinary, not only against the fixture, with the test skipped when the binary is absent. The corpus tests were confirmed to bite by perturbing kind inference, reference deduplication, and the column-shadowing check.Regular expressions took six review rounds, and the design changed twice
data-dict validates patterns with Rust's
regex. My first version validated with Python'sre, which is not a proxy for it in either direction:reaccepts lookaround and backreferences Rust rejects, and rejects\p{L},\zand POSIX classes Rust accepts. Each round of review found another divergence, and each targeted fix produced the next one.So it validates and matches with RE2, which shares Rust's finite-automata design and agrees with it on every case raised across those reviews. The first cut reached RE2 through the
duckdbalready in commons' dependencies; review then surfacedgoogle-re2, Google's own binding, maintained alongside RE2 itself and shipping wheels for the whole supported matrix. That deleted the shared connection, its lock, and the deferred import, along with the read-time coupling to DuckDB that an earlier version of this section flagged for reviewer judgement.Two constructs are known to differ, both verified against the binary as accepted by data-dict and refused here: extended mode
(?x)and CRLF-aware multiline(?R). Rust's named-group spelling(?<name>...)was a third, but google-re2's RE2 is newer than DuckDB's vendored copy and has accepted that spelling since 2023-07, so the divergence and its error-message hint are gone. The tests deliberately do not claim the list is exhaustive; an earlier version did and was wrong. Both remaining differences fail closed at construction with a message, and neither changes what a definition matches. Closing the gap needs a binding to Rust'sregex, and the only Python one is unmaintained.Departures from R
Integer literals are
intrather than the normalized string R keeps for 64-bit precision, with the i64 range check retained. Where R reaches PCRE for regexes, this uses RE2, so the two accept slightly different pattern sets at the edges described above.Verification: 483 tests pass, ruff and pyrefly clean.