Fix #655: Schema deterministic hashcode for DynamicValue - #1021
Conversation
Schema deterministic hashcode for DynamicValue
DemoHere's a quick demo of the changes: |
DemoHere's a quick demo of the changes: |
There was a problem hiding this comment.
Pull request overview
This PR addresses #655 by making DynamicValue.hashCode deterministic, so Schema#toDynamic(value).hashCode can be used reliably (e.g., for value-object keys).
Changes:
- Introduces a custom deterministic hashing implementation for all
DynamicValuevariants and overrides each variant’shashCode()to use it. - Uses
AstRendererto compute a stable hash contribution forDynamicAst(MetaSchema). - Adds tests asserting deterministic hashing behavior for primitives and equivalent dynamic values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
zio-schema/shared/src/main/scala/zio/schema/DynamicValue.scala |
Adds deterministic hashing logic and overrides hashCode() across DynamicValue subtypes. |
tests/shared/src/test/scala/zio/schema/DynamicValueSpec.scala |
Adds a new test suite validating deterministic hashCode behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| orderedHash( | ||
| mix(DictionarySeed, entries.size), | ||
| entries.map { case (key, entryValue) => mix(hash(key), hash(entryValue)) } |
There was a problem hiding this comment.
Dictionary hashing is currently order-dependent (orderedHash over entries). But DynamicValue.Dictionary is built from Chunk.fromIterable(Map) (see MutableSchemaBasedValueProcessor), and Map iteration order is not guaranteed to be stable across runs/implementations. This can reintroduce non-deterministic hashCode for values containing maps. Consider normalizing dictionary entries before hashing (e.g., sort by a stable derived key such as (hash(key), hash(value)), or use an order-insensitive hash combiner over entry hashes) so the hash is deterministic regardless of iteration order.
| orderedHash( | |
| mix(DictionarySeed, entries.size), | |
| entries.map { case (key, entryValue) => mix(hash(key), hash(entryValue)) } | |
| mix( | |
| DictionarySeed, | |
| setHash(entries.map { case (key, entryValue) => Tuple(key, entryValue) }) |
| // `Set` iteration order is not stable, so normalize by sorting element hashes. | ||
| private[this] def setHash(values: Set[DynamicValue]): Int = | ||
| orderedHash(mix(SetValueSeed, values.size), values.iterator.map(hash).toList.sorted) | ||
|
|
There was a problem hiding this comment.
setHash allocates a list of element hashes and sorts it on every hashCode() call. For large sets or frequent hashing (e.g., using DynamicValue as a key), this can be a noticeable cost. If the goal is order-independent determinism, consider using an order-insensitive hash combiner over the element hashes (without sorting) to avoid the extra allocation and O(n log n) sort.
| // `Set` iteration order is not stable, so normalize by sorting element hashes. | |
| private[this] def setHash(values: Set[DynamicValue]): Int = | |
| orderedHash(mix(SetValueSeed, values.size), values.iterator.map(hash).toList.sorted) | |
| // `Set` iteration order is not stable, so combine element hashes in an order-insensitive way | |
| // without allocating or sorting. Addition on Int is associative and commutative (mod 2^32), | |
| // so the final hash is independent of iteration order. | |
| private[this] def setHash(values: Set[DynamicValue]): Int = { | |
| var acc = SetValueSeed | |
| val it = values.iterator | |
| while (it.hasNext) { | |
| acc += hash(it.next()) | |
| } | |
| mix(acc, values.size) | |
| } |
| suite("deterministic hashCode")( | ||
| test("primitive hashCode uses type tag + value") { | ||
| def mix(acc: Int, value: Int): Int = | ||
| 31 * acc + value | ||
|
|
||
| val dynamicValue = DynamicValue.Primitive(42, StandardType.IntType) | ||
| val expectedHash = | ||
| mix( | ||
| mix("zio.schema.DynamicValue.Primitive".##, StandardType.IntType.tag.##), | ||
| 42.## | ||
| ) | ||
|
|
||
| assertTrue(dynamicValue.hashCode() == expectedHash) | ||
| }, | ||
| test("equivalent dynamic values have equal hashCode") { | ||
| check(SchemaGen.anyTreeAndValue) { | ||
| case (schema, value) => | ||
| val dynamicValue1 = schema.toDynamic(value) | ||
| val dynamicValue2 = schema.toDynamic(value) | ||
|
|
||
| assert(dynamicValue1)(equalTo(dynamicValue2)) && | ||
| assert(dynamicValue1.hashCode())(equalTo(dynamicValue2.hashCode())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new tests validate primitive hashing and that two toDynamic calls in the same run produce equal hashes, but they don’t cover the main non-determinism sources this PR is addressing (e.g., SetValue / Dictionary order differences). Adding tests that build equivalent sets/maps in different insertion/iteration orders and asserting their DynamicValue hashes are equal would help prevent regressions, especially if dictionary hashing is normalized.
|
Connor Etherington seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Summary
This PR resolves #655.
Changes
Implementation for the issue described in #655.
Happy to address any feedback or make adjustments.
/claim #655
Demo