feat(v3): N-block ORE comparator + ordered numeric & timestamptz#294
Merged
Conversation
…repo-wide) Behaviour-preserving rename of the self-contained eql_v3 SEM index-term type to its width-agnostic name, ahead of the N-block ORE comparator work. The eql_v2 public API (eql_v2.ore_block_u64_8_256, src/ore_block_u64_8_256/, v2 operators/docs/tests) is deliberately UNCHANGED. Scope (eql_v3 only): - src/v3/sem/ore_block_u64_8_256/ -> src/v3/sem/ore_block_256/ (dir + symbols) - eql-scalars Term::Ore ctor + REQUIRE paths; eql-codegen doc/assertion - committed codegen goldens (int2/int4/int8/date/text incl text_search) - eql-types: OreBlockU64_8_256 newtype -> OreBlock256, SQL constructor doc - tasks/pin_search_path.sql (v3 block only), splinter.sh (v3 rows only), clean_install_v3.sh, drop_operator_classes.sql (v3 opclass only) - v3 SQLx family tests (sem/mutations/inlinability), preserving the eql_v2.compare_ore_block_u64_8_256_terms parity reference in sem.rs - v3 docs (CLAUDE, adding-a-scalar, eql-functions, sql-support, analysis), CHANGELOG eql_v3-qualified mentions Covers the new old-name references introduced by #280 (text-search). Verified: codegen:parity (byte-for-byte), test:crates (fmt+clippy+tests), clean build + v3 release artifact carries no eql_v2 symbol. The DB-backed clean_install_v3 gate runs in CI (Docker unavailable locally). Deliberately NOT renamed: tests/sqlx/src/index_types.rs ORE_BLOCK_U64_8_256 (crypto wire index-term identifier, not the SQL type name).
The repo-wide rename sed over-rewrote a doc-comment reference to the v2 source file (src/ore_block_u64_8_256/functions.sql) that this v3 jsonb-only fork is a subset of. Restore the real v2 path (the v2 SEM dir keeps its name).
Derive the ORE block count N from term length instead of hardcoding 8, and wire the ordered numeric scalar while promoting timestamptz to ordered.
Direct comparator tests over generated fixtures: numeric terms are 702 bytes (N=14) and order across a full 14-value ascending chain spanning sign, magnitude, and fractional scale (so the left blocks decide ordering — the regression the missed 9 -> 1+n offset would fail); timestamptz terms are 604 bytes (N=12) and order 1900 < 2099. Verified end-to-end: numeric + timestamptz ordered matrix suites (211 each, incl. < <= > >= / ORDER BY / MIN / MAX) green; full SQLx suite 2026 passed; test:matrix:inventory reconciles both new ordered types; self-contained v3 install green.
…stamptz CHANGELOG: rewrite the timestamptz entry (ordering now ships), add the numeric ordered-domain entry, and add a Fixed entry for the N-block ORE comparator generalization + the eql_v3 ore_block_u64_8_256 -> ore_block_256 rename. Reference guide: document the fourth (numeric/Decimal) fixture discriminator — proc-macro routing, scalar_fixture arm, numeric_values accessor + distinctness guard, rust_decimal dep.
Complete the eql_v3.ore_block_u64_8_256 -> ore_block_256 rename across the docs (CLAUDE.md, the scalar guide, eql-functions, sql-support; eql_v2 names left unchanged) and the v3 SEM file header — the @file/subset comment had over-applied the rename to the v2-origin path (src/ore_block_u64_8_256). Regenerate the codegen reference goldens for every catalog type after rebasing onto eql_v3: add the numeric reference dir and expand timestamptz to its now-ordered shape so the parity gate passes. Also address review feedback: - ScalarKind::rust_type returns the now-real numeric type (rust_decimal::Decimal); only jsonb remains surfaceless. De-stale its doc and the 'timestamptz is equality-only' test comment; add numeric_maps_to_decimal. - Correct the guide's stale 'timestamptz is equality-only' prose and a dangling link to the removed design plan doc. - Add comparator_rejects_mismatched_block_widths (8-block vs 14-block terms must raise via the different-lengths guard). - Add the PR link (#276) to the numeric and N-block changelog entries.
has_ore_block_256 used "val ->> 'ob' IS NOT NULL", which stringifies a
scalar/object 'ob' and reports it present. ore_block_256 then fed the
malformed payload into jsonb_array_to_ore_block_256, which returns NULL
instead of raising, silently degrading a structurally invalid ORE term
into a NULL comparison/index term.
Tighten the guard to require a JSON array
(jsonb_typeof(val->'ob') = 'array'); a present-but-non-array 'ob' now
RAISEs at the extractor boundary. '{}' (absent ob) and '{"ob": null}'
(JSON null) remain absent (false). Adds T5 characterization cases for
the scalar/object 'ob' presence checks and the extractor RAISE.
Addresses CodeRabbit review thread on PR #276.
… fixture Strengthen the N-block ORE comparator tests so they run creds-free on no-creds CI shards, sourcing real ORE terms from committed fixtures: - assert_orders_like_oracle: all-pairs oracle agreement + antisymmetry (replaces the adjacent-pair-only ascending chain), with a row-count drift guard against the catalog fixture order - comparator_length_guard_sweep: boundary/off-by lengths for the 49*N+16 guard across N=1..14 - wide_block_term_compares_equal_to_itself: reflexive path at N=14/12 - numeric_scale_equivalents_collide: 1 == 1.0 ORE collision via the new hand-written v3_numeric_collision fixture (the value-equal pair the catalog distinctness guard forbids in eql_v2_numeric) Also fix the stale timestamptz comment in scalar_domains.rs (now ordered, not eq-only).
The eql-types crate landed on eql_v3 (PR #236) after this branch forked, so merging the base in surfaced a catalog_parity failure: CATALOG now has the numeric family and ordered timestamptz, but v3::all() didn't. - numeric.rs: four ordered domains (storage/_eq/_ord/_ord_ore), mirroring date.rs; numeric is the first scalar with a >8-block ORE term (14) - timestamptz.rs: add the two ordered domains; the eq-only/8-block-limit rationale is gone now that eql_v3.ore_block_256 derives N from term length - mod.rs: register the six new domains in all(), in CATALOG order - terms.rs: the ob term's SQL constructor is eql_v3.ore_block_256 (renamed this branch) and is width-agnostic (8/12/14 blocks) - v3_conformance.rs: cover numeric + timestamptz ord wire shapes; drop the stale equality-only claim - README: timestamptz no longer eq-only; add numeric
|
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 |
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
The
eql-v3-ore-block-256work, targetingeql_v3directly. Re-split from the accidental #276, which merged this branch intoeql-v3-ore-block-renamerather than landing each branch oneql_v3independently.eql_v3.ore_block_256derives block count from term length, no longer hardcoded to 8 blocks)timestamptz(12-block) to ordered domainsnumeric(14-block) domainsobpayloads at the extractor boundaryStacking
Stacked on #282 (
eql-v3-ore-block-rename → eql_v3, theore_block_u64_8_256 → ore_block_256SEM rename). This branch was rebased onto the rebased rename branch, which sits on currenteql_v3. Until #282 merges, this PR's diff includes the rename commits; it narrows to just the 256 work once #282 lands. Merge #282 first.Rebase adaptation
Rebased from the old
eql_v3base (858c751) onto currenteql_v3(postts-rs/schemarsbinding generation). The newnumeric+ orderedtimestamptzRust types gainedTS/JsonSchemaderives andschema()impls to match the rest of theeql_v3domain types, and the generatedbindings/+schema/were regenerated accordingly.Verification
mise run types:check— bindings/schema freshcargo test -p eql-types -p eql-scalars -p eql-codegen— all passcargo run -p eql-codegen— goldens unchanged