Support ordering comparison on comparable entity columns#203
Support ordering comparison on comparable entity columns#203alexander-yevsyukov wants to merge 5 commits into
Conversation
Store comparable message columns as order-preserving Datastore values, so `>`/`<`/`>=`/`<=` queries return semantically correct results: - add a `Duration` -> `LongValue(totalNanos)` mapping in `DsColumnMapping`; - encode any message marked with `(compare_by)` into an order-preserving key via the new `OrderedBytes` codec and `CompareByEncoder`, which reads the option from the message descriptor at runtime and reproduces the compiler-generated `compareTo` (nested messages, `Timestamp`/`Duration` and wrapper fields, and the `descending` flag); - route `(compare_by)` messages through `DsColumnMapping.ofMessage()`, leaving non-comparable messages on the existing string form. Both Datastore read paths (server-side `PropertyFilter` and the in-memory `ColumnPredicate`/sorting) compare the stored encoded value, so this makes the comparison correct on both. Bump CoreJvm to 2.0.0-SNAPSHOT.383, which introduces the comparable-field support in `Filters`. Add codec property tests (oracle: the generated `compareTo`), `DsColumnMapping` unit tests, and end-to-end emulator query tests covering `Duration`, `(compare_by)`, and `Timestamp` columns on both read paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Regenerated after bumping CoreJvm to 2.0.0-SNAPSHOT.383. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e86a8c9de
ℹ️ 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".
|
|
||
| private static final Map<Class<?>, ColumnTypeMapping<?, ? extends Value<?>>> defaults | ||
| = ImmutableMap.of(Timestamp.class, ofTimestamp(), | ||
| Duration.class, ofDuration(), |
There was a problem hiding this comment.
Keep existing Duration column values queryable
In deployments with rows written before this change, Duration columns fell through to ofMessage() and were stored as StringValues. Adding this default makes new writes and filters use a LongValue, so equality filters and column predicates built after upgrade no longer match those existing Datastore properties until every entity is rewritten. Please add an explicit migration/compatibility path instead of silently switching the persisted representation.
Useful? React with 👍 / 👎.
| if (CompareByEncoder.isComparable(msg.getDescriptorForType())) { | ||
| return StringValue.of(CompareByEncoder.encode(msg)); |
There was a problem hiding this comment.
Keep existing compare_by message keys queryable
Before this commit, even messages marked with (compare_by) were stored via Stringifiers.toString(msg), so existing entities already have that value in Datastore. This new branch encodes the same filter values as hex keys, which means equality filters and by-ID in-memory predicates over such columns miss pre-existing rows until they are rewritten. Please provide a migration or compatibility fallback for the old stringified representation.
Useful? React with 👍 / 👎.
| private static void encodeMessageValue(Message value, OrderedBytes bytes) { | ||
| var type = value.getDescriptorForType(); | ||
| if (isComparable(type)) { | ||
| encodeComparable(value, bytes); |
There was a problem hiding this comment.
Honor nested descending compare_by options
When a (compare_by) field is itself a comparable message whose own option has descending: true, this path appends the nested fields without applying that nested reversal; only top-level encode() calls invertAll(). The generated outer comparison compares the nested value by its compareTo(), so an outer column containing such a nested type sorts in the opposite order and Datastore >/< filters can return the wrong rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request updates Datastore column encoding so ordering comparison operators (>, <, >=, <=) behave correctly for additional “comparable” column types (notably Duration and messages marked with (compare_by)), by storing order-preserving values instead of relying on non-order-preserving stringified forms. It also includes a broader config/tooling bump (versions, wrapper, reports, and build tooling updates) accompanying the change.
Changes:
- Add order-preserving encoding for
(compare_by)message columns and storeDurationcolumns as total nanoseconds for correct ordering comparisons. - Add unit/property tests and emulator-backed integration tests to validate comparison behavior across server-side and in-memory read paths.
- Bump project/dependency/tooling versions and regenerate dependency reports; update Gradle wrapper and CI/version-guard workflows.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| version.gradle.kts | Bump published project version to 2.0.0-SNAPSHOT.215. |
| gradlew.bat | Gradle wrapper script text updates (config/tooling). |
| gradlew | Gradle wrapper script text updates (config/tooling). |
| gradle/wrapper/gradle-wrapper.properties | Gradle wrapper distribution URL update. |
| docs/dependencies/pom.xml | Regenerated dependency POM / version bumps. |
| docs/dependencies/dependencies.md | Regenerated dependency/license report for new versions. |
| datastore/src/test/proto/spine/test/datastore/comparison.proto | Test protos defining (compare_by) comparable messages and a record with comparable columns. |
| datastore/src/test/kotlin/io/spine/server/storage/datastore/query/ComparableColumnQuerySpec.kt | Emulator test covering ordering comparisons on Duration, (compare_by) message, and Timestamp columns. |
| datastore/src/test/kotlin/io/spine/server/storage/datastore/config/DsColumnMappingComparableSpec.kt | Tests for DsColumnMapping behavior for Duration, (compare_by) messages, and non-(compare_by) messages. |
| datastore/src/test/kotlin/io/spine/server/storage/datastore/config/CompareByEncoderSpec.kt | Property-style tests asserting encoded key order matches generated compareTo. |
| datastore/src/test/java/io/spine/server/storage/datastore/given/MeasurementStorage.java | Test storage wrapper for emulator-based querying. |
| datastore/src/test/java/io/spine/server/storage/datastore/given/MeasurementColumns.java | Comparable column definitions used in emulator tests. |
| datastore/src/main/java/io/spine/server/storage/datastore/config/OrderedBytes.java | New order-preserving byte accumulator, rendered as hex string. |
| datastore/src/main/java/io/spine/server/storage/datastore/config/DsColumnMapping.java | Route (compare_by) messages through order-preserving encoding; add Duration -> LongValue(nanos) mapping. |
| datastore/src/main/java/io/spine/server/storage/datastore/config/CompareByEncoder.java | New runtime (compare_by) encoder producing order-preserving string keys. |
| buildSrc/src/test/kotlin/io/spine/gradle/VersionGradleFileSpec.kt | Tests for parsing version.gradle.kts publishing version declarations. |
| buildSrc/src/test/kotlin/io/spine/gradle/VersionComparatorSpec.kt | Tests for semantic comparison of version strings. |
| buildSrc/src/test/kotlin/io/spine/gradle/report/pom/DependencyWriterSpec.kt | Tests for dependency-reporting behavior including resolved-version selection. |
| buildSrc/src/test/kotlin/io/spine/gradle/publish/MavenMetadataSpec.kt | Tests for Maven metadata XML round-trip behavior. |
| buildSrc/src/test/kotlin/io/spine/gradle/publish/IncrementGuardTest.kt | Tests for updated version-guard wiring/conditions. |
| buildSrc/src/test/kotlin/io/spine/gradle/fs/SpineTempDirSpec.kt | Tests for temp directory namespace and creation behavior. |
| buildSrc/src/test/kotlin/io/spine/gradle/fs/LazyTempPathSpec.kt | Tests for lazy temp-path creation and placement. |
| buildSrc/src/main/kotlin/write-manifest.gradle.kts | Task registration style adjustment for manifest exposure. |
| buildSrc/src/main/kotlin/uber-jar-module.gradle.kts | Publishing task dependency wiring adjustment. |
| buildSrc/src/main/kotlin/kmp-module.gradle.kts | KMP build logic tweaks (incl. Dokka handling and test task setup). |
| buildSrc/src/main/kotlin/jvm-module.gradle.kts | JVM module build logic tweaks (incl. Dokka handling and task registration). |
| buildSrc/src/main/kotlin/jacoco-kotlin-jvm.gradle.kts | Removed deprecated JaCoCo script plugin. |
| buildSrc/src/main/kotlin/jacoco-kmm-jvm.gradle.kts | Removed deprecated JaCoCo script plugin. |
| buildSrc/src/main/kotlin/io/spine/gradle/VersionGradleFile.kt | New implementation for reading/resolving version.gradle.kts values (incl. base-branch read). |
| buildSrc/src/main/kotlin/io/spine/gradle/VersionComparator.kt | New semantic version comparator used by reporting/selection logic. |
| buildSrc/src/main/kotlin/io/spine/gradle/report/pom/ProjectMetadata.kt | Gradle property delegation adjustments. |
| buildSrc/src/main/kotlin/io/spine/gradle/report/pom/DependencyWriter.kt | Dependency report now prefers versions selected by Gradle resolution. |
| buildSrc/src/main/kotlin/io/spine/gradle/publish/MavenMetadata.kt | New Maven metadata model for version checks. |
| buildSrc/src/main/kotlin/io/spine/gradle/publish/IncrementGuard.kt | Updated guard wiring and base-branch comparison gating. |
| buildSrc/src/main/kotlin/io/spine/gradle/publish/CheckVersionIncrement.kt | Expanded version checks (base-branch increment + not-published). |
| buildSrc/src/main/kotlin/io/spine/gradle/kotlin/KotlinConfig.kt | Opt-in flags composition adjusted (JVM-only ExperimentalPathApi). |
| buildSrc/src/main/kotlin/io/spine/gradle/fs/SpineTempDir.kt | New shared per-JVM temp directory with shutdown cleanup. |
| buildSrc/src/main/kotlin/io/spine/gradle/fs/LazyTempPath.kt | Temp directory creation now nests under SpineTempDir. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/QueryDsl.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/PostgreSql.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/MySql.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/HsqlDb.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/Hikari.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/storage/H2.kt | New storage dependency coordinates holder. |
| buildSrc/src/main/kotlin/io/spine/dependency/local/Logging.kt | Bump local logging dependency version constant. |
| buildSrc/src/main/kotlin/io/spine/dependency/local/CoreJvm.kt | Bump Core JVM dependency version constant. |
| buildSrc/src/main/kotlin/io/spine/dependency/local/Compiler.kt | Bump compiler fallback versions. |
| buildSrc/src/main/kotlin/io/spine/dependency/Dependency.kt | Add Configuration.isDokka helper. |
| buildSrc/src/main/kotlin/io/spine/dependency/boms/BomsPlugin.kt | Exclude Dokka configurations from version/BOM forcing logic. |
| buildSrc/src/main/kotlin/dokka-setup.gradle.kts | Disable Dokka Javadoc publication for KMP modules. |
| buildSrc/src/main/kotlin/DependencyResolution.kt | Exclude Dokka configurations from forced versions. |
| .github/workflows/revalidate-versions.yml | New workflow to revalidate open PRs when base branch version advances. |
| .github/workflows/publish.yml | Add clearer error reporting on publish failures. |
| .github/workflows/increment-guard.yml | Enhance version-guard workflow (base fetch, status publishing, event coverage). |
| .claude/settings.json | Update Claude Code permissions list. |
| private static void encodeFieldPath(Message message, String path, OrderedBytes bytes) { | ||
| List<String> names = PATH_SPLITTER.splitToList(path); | ||
| var current = message; | ||
| for (var i = 0; i < names.size() - 1; i++) { | ||
| var field = fieldNamed(current.getDescriptorForType(), names.get(i)); | ||
| current = (Message) current.getField(field); | ||
| } | ||
| var leaf = fieldNamed(current.getDescriptorForType(), names.get(names.size() - 1)); | ||
| encodeValue(leaf, current.getField(leaf), bytes); | ||
| } |
| /** | ||
| * Appends the UTF-8 bytes of the given string in a self-delimiting, order-preserving form. | ||
| * | ||
| * <p>UTF-8 byte order equals Unicode code point order, which matches | ||
| * {@link String#compareTo(String) String.compareTo()} for all Basic Multilingual Plane text. | ||
| * The two orderings can differ only for strings containing supplementary (non-BMP) characters, | ||
| * which {@code String.compareTo()} orders by UTF-16 code unit. | ||
| */ | ||
| void putString(String value) { | ||
| var utf8 = value.getBytes(UTF_8); |
|
Closing as a failing experiment. |
What
Makes the Datastore storage store comparable message columns as order-preserving values, so
>/</>=/<=queries and filters on such columns return semantically-correct results.TimestampTimestampValueVersionLongValue(number)DurationLongValue(totalNanos)(compare_by)messageStringWhy
CoreJvm
2.0.0-SNAPSHOT.383(consumed here) introducesComparable-field support in theFiltersordering comparisons. The underlying machinery lives inbase-libraries: the(compare_by)proto option (makes a messageComparable) andio.spine.compare.ComparatorRegistry(supplies aComparatorfor types likeTimestamp/Durationthat do not implementComparable).Both Datastore read paths — the server-side
PropertyFilterpush and the in-memoryColumnPredicate/sorting — compare the value produced byDsColumnMapping, never the original message.DsColumnMappingpreviously only order-preservedTimestampandVersion; every other message column fell through to a stringified form whose lexicographic order does not match semantic order, so ordering queries on aDurationor a custom(compare_by)column returned wrong results.A bare
Comparatorcan't yield an absolute sort key, but the(compare_by)option is runtime-readable from the message descriptor (OptionsProto.compareBy, no source retention — the proto comment invites "runtime comparators … using the reflection API"), so the ordering can be reconstructed from structure.How
OrderedBytes— an order-preserving byte codec (sign-flipped integrals, IEEE-754doubletransform, self-delimiting UTF-8 strings, whole-key inversion fordescending) rendered as a hexString, so all three read paths compare it correctly (aBlobwould break the in-memoryComparableValueExtractor, which handlesSTRING/LONG/DOUBLE/BOOLEAN/TIMESTAMP).CompareByEncoder— readsOptionsProto.compareByfrom the descriptor and encodes the fields in declared order, recursing into nested(compare_by)messages,Timestamp/Duration, and well-known wrapper fields, and honoring thedescendingflag — reproducing the compiler-generatedcompareTo.DsColumnMapping— adds theDuration -> LongValue(totalNanos)mapping and routes(compare_by)messages through the encoder inofMessage(); non-comparable messages keep the previous string form. ExistingTimestamp/Versionencodings are unchanged (backward compatible).CoreJvm2.0.0-SNAPSHOT.381 -> .383.Testing
24 new tests, all green (full
clean buildpasses):CompareByEncoderSpec) — for many generated value pairs, assert the encoded-key order matches the message's own generatedcompareTo(single/multi-field,descending,Timestamp/Duration, nested,double/bool/enum/wrapper, dotted path).DsColumnMappingComparableSpec) —Duration -> LongValue(nanos),(compare_by) -> StringValue, non-(compare_by) ->string form.ComparableColumnQuerySpec,@EmulatorTest) — store records and assert exact matches for>/</>=/<=+ a range onDuration,(compare_by), andTimestampcolumns, across both the server-side and the in-memory (by-IDs) read paths.Reviewer notes
2.0.0-SNAPSHOT.215. Dependency reports regenerated for the.383bump.Update configcommit (config-submodule float) that is unrelated to this feature.ComparatorRegistrythat is neither structurally known (Timestamp/Duration/wrappers) nor(compare_by)still needs acustomMapping()— a bare comparator has no absolute sort key.🤖 Generated with Claude Code