refactor: value Int of int -> Int64 of int64 - #19
Conversation
Ordinary integers (entity ids in value position, tx ids, counts, journal days, epoch-ms numerics) now use Int64 of int64 instead of Int of int, eliminating 32-bit int overflow on Melange and removing the misuse of Instant for plain integers. - type: Int of int -> Int64 of int64; QueryFormInt of int -> int64 - equality/compare: Int64 vs Float numeric ordering, Int64 vs Instant comparable by magnitude but never equal (distinct semantics) - hashing: murmur3_hash_long for Int64 payloads - built-ins: exact int64 arithmetic; number?/integer? reject Instant; range emits int64 lists - parser: EDN ints parse via Int64.of_string; #inst -> Instant - codecs (sqlite + melange): Int64 -> Transit.Int64 unconditionally, Instant -> Transit.Date; decode accepts Int/Int64/Big_int and ~m tags for backwards compatibility - storage: normalize_stored_datom migrates legacy Instant values to Int64 unless the attr is db.type/instant (real dates preserved) - js facade: Yojson ints/Intlit -> Int64; Instant still prints as #inst/Intlit
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c39cad18a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | _, Instant millis -> | ||
| (* older databases stored plain ints as Instant; only db.type/instant | ||
| attrs are real dates *) | ||
| { datom with v = Int64 millis } |
There was a problem hiding this comment.
When a stored database contains a genuine #inst/Instant value on an untyped attribute, this catch-all arm silently rewrites it to Int64. Such values are accepted by transactions, and the new codecs encode them as Transit.Date, but after decoding, every subsequent restore still changes their type; a database written by this commit therefore cannot round-trip dates unless every date attribute has an explicit db.type/instant schema. Preserve decoded dates and gate any legacy magnitude-based migration with storage-format metadata rather than attribute typing.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
| let result = List.fold_left (fun acc item -> acc /. float_of_numeric item) (float_of_numeric first) rest in | ||
| let integral = Float.is_integer result in | ||
| Some (numeric_result (has_float || not integral) result) | ||
| if has_float || not integral then | ||
| Some (Float result) | ||
| else | ||
| Some (Int64 (Int64.of_float result)) |
There was a problem hiding this comment.
When all operands are newly supported Int64 values above the IEEE-754 exact range, this converts them to floats before deciding whether the quotient is integral, losing bits and potentially returning the wrong value and constructor. For example, (/ 9007199254740993 3) operates on the rounded 9007199254740992.0 and returns a fractional Float instead of Int64 3002399751580331; perform exact int64 division when the integer operands divide evenly, falling back to floating-point only for fractional results.
Useful? React with 👍 / 👎.
| let rec json_of_value = function | ||
| | Nil -> `Null | ||
| | Int value -> `Int value | ||
| | Int64 value -> `Intlit (Int64.to_string value) |
There was a problem hiding this comment.
When q, pull, or datoms returns an Int64 outside JavaScript's safe-integer range, Intlit is emitted as an unquoted JSON number and js_of_json immediately feeds it to JSON.parse, which silently rounds it. For example, an internal Int64 9007199254740993L reaches callers as the JavaScript number 9007199254740992, defeating the new exact representation at the JS API boundary; unsafe values need a non-Number representation such as BigInt or a lossless tagged/string encoding.
Useful? React with 👍 / 👎.
| let minute = Int64.to_int (Int64.div (Int64.rem rem 3600000L) 60000L) in | ||
| let second = Int64.to_int (Int64.div (Int64.rem rem 60000L) 1000L) in | ||
| let ms = Int64.to_int (Int64.rem rem 1000L) in | ||
| Printf.sprintf "%04d-%02d-%02dT%02d:%02d:%02d.%03dZ" year month day hour minute second ms |
There was a problem hiding this comment.
For valid instant values outside years 0000–9999, this formatter emits a variable-width unsigned year that the repository's own parse_instant_millis rejects because it requires the first hyphen at offset 4. For example, Instant 253402300800000L prints readably as #inst "10000-01-01T00:00:00.000Z", but feeding that form back to read_edn raises invalid #inst literal; negative years are malformed similarly. Format extended years in the signed six-digit ISO form and teach the reader to accept it, or reject unsupported values before claiming a readable representation.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
Upstream -datoms/-seek-datoms/-rseek-datoms/-index-range/find-datom all run validate-indexed: an :avet lookup on an attr that is not a ref, unique, or :db/index true raises "Attribute :x should be marked as :db/index true". Our datoms/datoms_list/fold_datoms/find_datom skipped the check (the stale comment claimed d/datoms is ISearch/-search; it is not). Internal paths that mirror -search keep the unvalidated search_datoms and now also follow upstream's fallback: an attr+value pattern on a non-indexed attr scans aevt filtered by value instead of probing avet.
- entity materialization reads only the entity's own eavt datoms since 9284149; the scan counter assertion still expected one all-datoms scan. - the shared-tempids test listed expected datoms in insertion order, but datoms :eavt returns attr-sorted results (friend < name < parent).
1 participant
Summary
Datascript.value's plain-integer constructor changes fromInt of inttoInt64 of int64. Melange/JSintis 31-bit, so epoch-millis and large counts overflowed; the old code worked around this by stuffing big ints intoInstant of int64, which conflated integers with real dates. This PR makesInt64the single ordinary-integer representation and restoresInstantto mean onlydb.type/instant/#inst/Datevalues.This branch also fixes two pre-existing test failures on
main(see "Test fixes" below).Semantics
Int64 of int64: every ordinary integer — counts, journal days, epoch-ms numerics, integer literals.entity_id/txstay OCamlintinternally; values produced from them areInt64.Float of float: true floats only.Instant of int64: only real instants — from#instEDN,Transit.Date/~mtags,db.type/instantschema attrs,squuid-related paths. Never chosen by magnitude.value_equal:Int64 1 <> Float 1.0;Int64 x <> Instant x(never equal despite same payload).compare_value: Int64/Float/Ref order numerically; Instant↔numeric comparisons order by magnitude (upstreamvalueOfbehavior) but with distinct type rank.Int64hashes viamurmur3_hash_long;number?/integer?rejectInstant;even?/odd?/zero?/pos?/neg?still accept it (upstream compat).Floatoperand is present ((+ 1 2)→Int64, notFloat);count/count-distinctreturnInt64.Wire / codec
Int64 → Transit.Int64unconditionally (no "fits in int" downgrade);Instant → Transit.Date;Ref → Transit.Int.Transit.Int | Int64 | Big_int → Int64;Transit.Dateand~m/Tagged("m")→Instant.`Int/`Intlit→Int64;Int64 →`Intlit`` unconditionally.int(entity ids, pull limits,rand-intbounds) go throughUtil.int64_to_int/int64_to_int_exn— explicit, range-checked, never silent truncation.Storage compatibility / migration
No format version bump needed. On-disk datoms decode
Int64/Int/Big_intuniformly toInt64.normalize_stored_datom(schema-aware, runs on restore) migrates legacy values:Instantunder a non-db.type/instantattr →Int64(the historical mis-encoding of plain ints).Int64under adb.type/instantattr →Instant.Instantunderdb.type/instant→ preserved.Int64underdb.type/refattr →Ref(range-checked).Test fixes (pre-existing
mainfailures)test_datascript"AVET datoms reject untyped ref-valued attrs"): upstream-datoms/-seek-datoms/-rseek-datoms/-index-range/find-datomall runvalidate-indexed— an:avetlookup on an attr that isn't a ref, unique, or:db/index trueraises"Attribute :x should be marked as :db/index true".Db.datoms/datoms_list/fold_datoms/find_datomskipped that check (a stale comment claimedd/datomsisISearch/-search; it isn't).datomsnow validates; internal-searchmirrors (entity/pull reverse-attr lookups, query fast paths) use unvalidatedsearch_datomsand follow upstream's fallback of an aevt scan filtered by value when the attr isn't indexed.test_entity"full entity materialization may scan all datoms once"): stale expectation. Since 9284149,entity_attrsmaterializes forward attrs only via the per-entity eavt lookup — matching upstreamtouch(-search db [eid]) — so the all-datoms counter stays at 0.test_datascript"value-position tempids share the entity-id tempid's allocation", masked behind the AVET failure): the expected list was written in insertion order, butdatoms :eavtreturns attr-sorted results (friend < name < parent).Tests
New
test/test_int64.mlcovers: 1_700_000_000_000L roundtrip (would overflow 32-bit), int64 min/max boundaries, epoch-ms not becomingInstant,#inst↔Instantboth directions,Int64↔Floatequality/order/hash semantics,number?/integer?/zero/even/odd predicates, query range filters/max/lookup-ref on int64, transit codec encode/decode both directions incl. legacy tags, and end-to-end kvs restore of the legacy-Instant→Int64migration. Native + jsoo/melange smoke + upstream-cljs cross-runtime parity all pass;dune runtestis fully green on this branch.Build/test:
dune build @install;UPSTREAM_DATASCRIPT_REPO=... UPSTREAM_DATASCRIPT_JS=... dune runtest(see README/blueprint for the upstream bundle build).Link to Devin session: https://app.devin.ai/sessions/035c4c03c3a146fc9ff59709d3931abb
Open in Devin Desktop: https://app.devin.ai/desktop/session/035c4c03c3a146fc9ff59709d3931abb?variant=devin
Requested by: @RCmerci