refactor(eql-domains): move kind/fixtures off DomainFamily into TypeFixtures records - #326
Merged
Merged
Conversation
…ixtures records
Slim DomainFamily to structural { name, domains }; relocate the fixture
vocabulary (ScalarKind, BoundedIntKind, Fixture, fixtures!/int_values!/
text_values!, *_VALUES) into a fixtures module, and pair each catalog row with
its kind+values via a per-type TypeFixtures record + FIXTURES table. The old
struct-enforced 1:1 is replaced by a compile-time const parity block
(FIXTURES mirrors CATALOG by name+order, checked on every build).
Behaviour-preserving: generated SQL/TS/JSON and dump-catalog are
byte-identical; no fixture value changes. Supersedes the spec's eql-fixtures
crate extraction and the earlier fields-on-struct draft.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… kind guard, doc fix - Add str_eq_tests::str_eq_matches_iff_byte_identical: the parity guard's only new logic was exercised solely on the matching path against aligned data, so a str_eq bug (true on length-equal-but-differing bytes) would silently neuter it. Pin equal/length-diff/one-byte-diff cases. (#2) - Add catalog_tests::every_record_kind_matches_its_family: a catalog-wide family.name -> kind guard over EVERY record (the parity block checks only name+length, so a swapped kind like { family: &INT8, kind: I16 } would otherwise pass). Extends the int-only test to all 10 kinds. (#1) - Align ScalarKind::Timestamptz doc with Date: accessors are unreachable by construction (as_bounded_int() -> None), not 'panic'. (#3)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/eql-domains/src/fixtures/kind.rs`:
- Around line 5-8: Update the documentation in Kind’s fixture comments to
enumerate all non-integer variants now present in the enum. The current
docstrings on the Kind accessors and related comments still mention only
Numeric/Text/Jsonb/Date or “the others”; revise those descriptions to include
Timestamptz, Bool, F32, and F64 as non-integer kinds, keeping the wording
exhaustive and consistent with Kind and its fixed-width integer accessors.
In `@crates/eql-domains/src/tests.rs`:
- Around line 879-907: Move the `family.name` ↔ `kind` invariant out of
`every_record_kind_matches_its_family` and enforce it in the fixture layer so
`fixtures_for_token(token).kind` cannot diverge at compile time. Update the
`TypeFixtures` construction/parity path in the fixtures definitions to either
derive `kind` directly from `family` or add a const-time check that binds them
together, then keep the test as a secondary safety net if needed. Focus on the
`TypeFixtures` records and the macro-facing lookup used by
`crates/eql-tests-macros/src/lib.rs` so mismatches are caught before expansion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84c5060c-883b-4722-b83a-9665b6f9c8cc
📒 Files selected for processing (10)
crates/eql-domains/src/fixture.rscrates/eql-domains/src/fixtures/fixture.rscrates/eql-domains/src/fixtures/kind.rscrates/eql-domains/src/fixtures/mod.rscrates/eql-domains/src/fixtures/record.rscrates/eql-domains/src/fixtures/values.rscrates/eql-domains/src/lib.rscrates/eql-domains/src/tests.rscrates/eql-tests-macros/src/lib.rstests/sqlx/src/scalar_domains.rs
💤 Files with no reviewable changes (1)
- crates/eql-domains/src/fixture.rs
Comment on lines
+5
to
+8
| /// The fixed-width integer kinds — exactly those scalar kinds with an `i128` | ||
| /// range and `MIN`/`MAX`/`Zero` sentinels. These accessors are **total**: every | ||
| /// variant answers every method. Non-integer kinds (`Numeric`/`Text`/`Jsonb`/ | ||
| /// `Date`) are simply not representable here, so there is no partial function to |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the non-integer kind docs exhaustive.
The docs still describe non-integers as only Numeric/Text/Jsonb/Date or “the others” as Numeric/Text/Jsonb, but the enum now includes Timestamptz, Bool, F32, and F64 too.
Suggested doc update
-/// variant answers every method. Non-integer kinds (`Numeric`/`Text`/`Jsonb`/
-/// `Date`) are simply not representable here, so there is no partial function to
+/// variant answers every method. Non-integer kinds (`Numeric`/`Text`/`Jsonb`/
+/// `Date`/`Timestamptz`/`Bool`/`F32`/`F64`) are simply not representable here,
+/// so there is no partial function to
@@
-/// the others (`Numeric`/`Text`/`Jsonb`) have string fixtures and no numeric
-/// range — though `Numeric`/`Text` are still ORE-orderable, only `Jsonb` is not.
+/// non-integer kinds have no fixed-width integer range. `Numeric`/`Text`/
+/// `Date`/`Timestamptz`/`F32`/`F64` are ordered where their catalog rows include
+/// ORE, `Jsonb` is not ordered, and `Bool` is storage-only.Also applies to: 19-21
🤖 Prompt for 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.
In `@crates/eql-domains/src/fixtures/kind.rs` around lines 5 - 8, Update the
documentation in Kind’s fixture comments to enumerate all non-integer variants
now present in the enum. The current docstrings on the Kind accessors and
related comments still mention only Numeric/Text/Jsonb/Date or “the others”;
revise those descriptions to include Timestamptz, Bool, F32, and F64 as
non-integer kinds, keeping the wording exhaustive and consistent with Kind and
its fixed-width integer accessors.
Comment on lines
+879
to
+907
| /// Catalog-wide `family.name` ↔ `kind` guard over EVERY record, not just the | ||
| /// integer ones. `TypeFixtures` carries `family` and `kind` as independent | ||
| /// fields, and the compile-time parity block checks only `family.name` + | ||
| /// length — so `TypeFixtures { family: &INT8, kind: I16, .. }` would compile | ||
| /// and the parity block would pass. This test is the single guard that binds | ||
| /// every record's `kind` to the kind its family is supposed to map onto, for | ||
| /// all kinds (the non-integer kinds are otherwise only checked by individual | ||
| /// `*_spec` tests). A mismatched or swapped `kind` fails here. | ||
| #[test] | ||
| fn every_record_kind_matches_its_family() { | ||
| for rec in FIXTURES { | ||
| let expected = match rec.family.name { | ||
| "int2" => ScalarKind::I16, | ||
| "int4" => ScalarKind::I32, | ||
| "int8" => ScalarKind::I64, | ||
| "date" => ScalarKind::Date, | ||
| "timestamptz" => ScalarKind::Timestamptz, | ||
| "numeric" => ScalarKind::Numeric, | ||
| "text" => ScalarKind::Text, | ||
| "bool" => ScalarKind::Bool, | ||
| "float4" => ScalarKind::F32, | ||
| "float8" => ScalarKind::F64, | ||
| other => panic!("unmapped scalar token {other} in FIXTURES"), | ||
| }; | ||
| assert_eq!( | ||
| rec.kind, expected, | ||
| "{} record carries the wrong kind", | ||
| rec.family.name | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Promote the family↔kind check out of the test suite.
This test is currently the only place that binds TypeFixtures.kind to family.name, but the production predicates in crates/eql-tests-macros/src/lib.rs now branch on fixtures_for_token(token).kind. That means a mismatched kind still compiles, passes the const parity block, and changes macro expansion unless this test suite runs. Please move this invariant into the fixture-layer const parity guard, or derive kind from family instead of storing it twice.
🤖 Prompt for 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.
In `@crates/eql-domains/src/tests.rs` around lines 879 - 907, Move the
`family.name` ↔ `kind` invariant out of `every_record_kind_matches_its_family`
and enforce it in the fixture layer so `fixtures_for_token(token).kind` cannot
diverge at compile time. Update the `TypeFixtures` construction/parity path in
the fixtures definitions to either derive `kind` directly from `family` or add a
const-time check that binds them together, then keep the test as a secondary
safety net if needed. Focus on the `TypeFixtures` records and the macro-facing
lookup used by `crates/eql-tests-macros/src/lib.rs` so mismatches are caught
before expansion.
…e time Address PR review: - Hoist the family.name <-> kind invariant into the compile-time parity block (kind_tag/expected_kind in fixtures/record.rs) so a mismatched TypeFixtures.kind fails the eql-domains build before any consumer (incl. eql-tests-macros expansion) reads FIXTURES; keep the runtime test as a secondary safety net. - Make the ScalarKind/BoundedIntKind non-integer-kind docs exhaustive (add Timestamptz/Bool/F32/F64; fix stale ORE-orderability wording). - Fix stale BOOL.fixtures -> BOOL_FIXTURES.values doc comment.
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.
Summary
Slims
eql_domains::DomainFamilyto a purely-structural{ name, domains }by moving thekindandfixturesconcern into a dedicated fixture-layer record (TypeFixtures), keeping one crate with a clean one-wayfixtures → catalogdependency, and preserving byte-for-byte output (SQL / TS / JSON /dump-catalog).DomainFamilyis now exactly{ name: &'static str, domains: &'static [Domain] }—kind/fixturesremoved as fields.fixturesmodule owns the fixture vocabulary:ScalarKind,BoundedIntKind,Fixture, thefixtures!/int_values!/text_values!macros, the*_VALUESconsts, and a per-typeTypeFixtures { family: &DomainFamily, kind, values }record (INT4_FIXTURES…FLOAT8_FIXTURES) plus aFIXTUREStable mirroringCATALOGorder.kind/fixturesis replaced by a compile-timeconstparity block infixtures/record.rs(assert!(FIXTURES.len() == CATALOG.len())+ name/order check via aconst fn str_eq). It const-evaluates on everycargo buildand fails the build witherror[E0080]on a missing/misaligned record — it cannot be#[cfg]-gated or filtered out, unlike a runtime test..kind/.fixturesreaders rewired:eql-domainstests,eql-tests-macroskind-predicates (newfixtures_for_token), andtests/sqlx/src/scalar_domains.rsmacros/inline readers.All public crate-root paths are preserved via
pub use fixtures::{…}re-exports;*_VALUESkeep identical names and types. PR 4 (bindings) reads onlyname/domains/terms/ENVELOPE_KEYS, so it is unaffected.This supersedes the spec's separate
eql-fixturescrate extraction (a crate adds a manifest/build-edge and cycle risk for no benefit, since the dependency is one-way) and the earlier fields-on-DomainFamilydraft (those fields are removed, not retained).Scope of the const block (no overclaim)
The
const _block proves name + order coverage only. Fixture-VALUE correctness stays gated by the in-crate value/invariant tests (values_tests, bounds/distinctness/pivot/kind-pairinginvariant_tests/float_tests/catalog_tests) plus a verbatim-move literal diff.Verification (all DB-free / creds-free)
mise run test:crates— fmt + clippy-D warnings+ tests for all four lean crates ✅mise run codegen:parity— generated SQL byte-identical to committed references ✅mise run types:check— TS bindings + JSON schemas, no git diff ✅mise run test:matrix:inventory— tests/sqlx matrix compiles with rewired macros; catalog reconciled ✅cargo run -p eql-codegen -- dump-catalog | shasum— digest identical pre/post ✅mise run test:matrix:expand—{int4,text,bool}_expanded.rssnapshots stable (macro emits byte-identical tokens) ✅Notes
dump-catalogbyte-identical), so per the changelog discipline noCHANGELOG.mdentry is added.eql-tests-macros'sunknown_token_fails_loudly#[should_panic]message updated fromCATALOGtoFIXTURES—is_temporal_tokennow legitimately panics via theFIXTURESlookup; the loud-failure behavior is preserved.Summary by CodeRabbit
Refactor
Bug Fixes
Tests