refactor(eql-scalars): split lib.rs into definitions + impl/test modules

[tobyhede]

What

crates/eql-scalars/src/lib.rs had grown to 1215 lines — ~55% of it tests, with each enum's definition interleaved with its impl block. This reorganises the crate so the catalog reads top-to-bottom, per the stated ideal: definitions at the top level for simple access; implementation and tests in modules.

Pure code-move, zero behaviour change. The crate's public API surface is unchanged.

Layout

lib.rs (1215 → ~240 lines) now holds only the definitions:

• the six type defs (BoundedIntKind, ScalarKind, Term, Fixture, DomainSpec, ScalarSpec)
• the two crate-internal macros (fixtures!, int_values!)
• all catalog data (ORDERED_INT_DOMAINS, INT*_FIXTURES, INT4/INT2/INT8/DATE, CATALOG, INT*_VALUES)

Inherent impl blocks move to sibling modules (methods travel with their types — no re-exports needed):

File	Impls
kind.rs	BoundedIntKind, ScalarKind
term.rs	Term (both blocks)
fixture.rs	Fixture
spec.rs	ScalarSpec

The 7 unit-test modules move verbatim into tests.rs (#[cfg(test)] mod tests), kept as one group because rust_tests spans multiple types; use super::* → use crate::*. Test names are preserved (now under the tests:: path).

Why these choices

• Inherent impls in submodules is free in Rust — no pub use needed, public paths (eql_scalars::{CATALOG, INT4_VALUES, …}) unchanged, so eql-codegen / eql-tests-macros / tests/sqlx compile without edits.
• Macros stay in lib.rs (crate-internal, not #[macro_export]) so they precede their use sites; tests.rs sees fixtures! by textual scoping (mod tests; is declared after the macro).
• Tests as one group, not co-located, because rust_tests exercises BoundedIntKind + ScalarKind + Fixture + ScalarSpec and would otherwise fragment.

Verification

Step	Result
cargo test -p eql-scalars	✅ 40 tests pass
mise run test:codegen	✅ pass
mise run codegen:parity	✅ byte-for-byte identical int4 golden — proves catalog data unchanged
mise run test:crates (clippy -D warnings)	✅ clean
eql-codegen / eql-tests-macros build	✅ unchanged
mise run test:matrix:inventory	⚠️ not run locally — needs CipherStash creds to generate gitignored fixtures; environmental, exercises harness wiring untouched here. Run in CI.

The byte-for-byte parity pass is the definitive proof this is behaviour-preserving.

Scope

Diff touches crates/eql-scalars/src/ only: lib.rs (−963/+39) + 5 new module files.

Summary by CodeRabbit

• Refactor

  • Reorganized internal scalar and term catalog definitions by moving implementations to dedicated modules for improved code organization and maintainability.
  • Relocated accessor methods and domain-related helpers to specialized modules.

• Tests

  • Added comprehensive unit test suite validating scalar and term contracts, fixture behavior, operator unions, and catalog consistency.
  • Tests ensure domain structure integrity and enforce fixture-value invariants.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 54a43f19-f09f-4ce9-af3c-99b6ed4b7aef

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v3-eql-scalars-reorg

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/eql-scalars/src/tests.rs (1)

606-625: 💤 Low value

Outdated comment: CATALOG now includes DATE.

The comment on line 607 states "CATALOG is int-only, so the Str path is otherwise unexercised", but DATE is now in the catalog with Fixture::Date(...) variants that do exercise the DistinctKey::Str arm in fixture_values_are_distinct_by_resolved_number.

📝 Suggested comment update

 #[test]
 fn distinct_key_separates_string_fixtures() {
-    // CATALOG is int-only, so the `Str` path is otherwise unexercised.
+    // Exercises the `Str` path for string-backed variants beyond DATE (which is
+    // in the catalog but only uses Date fixtures).
     assert_eq!(

🤖 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-scalars/src/tests.rs` around lines 606 - 625, Update the outdated
test comment in the distinct_key_separates_string_fixtures test: remove or
reword the sentence "CATALOG is int-only, so the `Str` path is otherwise
unexercised" to reflect that CATALOG now includes DATE and that Fixture::Date
variants exercise the DistinctKey::Str arm (see
fixture_values_are_distinct_by_resolved_number and DistinctKey::Str). Ensure the
comment accurately describes current coverage (e.g., note DATE exercises the Str
path) or drop the claim entirely so the test comment matches the code behavior
in distinct_key_separates_string_fixtures and related fixtures.

🤖 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-scalars/src/kind.rs`:
- Around line 89-99: The docstring for ScalarKind::rust_type incorrectly claims
the returned strings are "Rust type name[s] as they appear in generated source"
while Numeric/Text/Jsonb return "numeric"/"text"/"jsonb" and are not used by
codegen; update the ScalarKind::rust_type documentation to accurately state its
behavior (e.g., that it returns canonical Rust type names only for
I16/I32/I64/Date and returns SQL/type-identifiers or placeholders for
Numeric/Text/Jsonb), and mention the only call sites
(crates/eql-scalars/src/tests.rs) if helpful; alternatively, if you prefer
functional change, either remove/merge the unused variants from ScalarKind or
change rust_type to return Option<&'static str> for only codegen-supported
variants—apply the chosen change to ScalarKind::rust_type and update any tests
that rely on its current return values.

---

Nitpick comments:
In `@crates/eql-scalars/src/tests.rs`:
- Around line 606-625: Update the outdated test comment in the
distinct_key_separates_string_fixtures test: remove or reword the sentence
"CATALOG is int-only, so the `Str` path is otherwise unexercised" to reflect
that CATALOG now includes DATE and that Fixture::Date variants exercise the
DistinctKey::Str arm (see fixture_values_are_distinct_by_resolved_number and
DistinctKey::Str). Ensure the comment accurately describes current coverage
(e.g., note DATE exercises the Str path) or drop the claim entirely so the test
comment matches the code behavior in distinct_key_separates_string_fixtures and
related fixtures.

🪄 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: 0346b025-8b01-4f9b-a22e-1e89b430f602

📥 Commits

Reviewing files that changed from the base of the PR and between 6a95ebc and c919b61.

📒 Files selected for processing (6)

• crates/eql-scalars/src/fixture.rs
• crates/eql-scalars/src/kind.rs
• crates/eql-scalars/src/lib.rs
• crates/eql-scalars/src/spec.rs
• crates/eql-scalars/src/term.rs
• crates/eql-scalars/src/tests.rs

[auxesis]

Love to see this re-org and cleanup, thanks @tobyhede.