Fix non-deterministic feedback deduplication in ClickHouse#7077
Open
AntoineToussaint wants to merge 2 commits intomainfrom
Open
Fix non-deterministic feedback deduplication in ClickHouse#7077AntoineToussaint wants to merge 2 commits intomainfrom
AntoineToussaint wants to merge 2 commits intomainfrom
Conversation
The ROW_NUMBER() window function used `ORDER BY timestamp DESC` to pick the latest feedback per target_id, but `timestamp` is a DateTime (second-precision) column materialized from UUIDv7. When multiple feedback records share the same second, the ordering is undefined, causing flaky test failures (e.g. getting value 1.0 instead of 5.0). Add `id DESC` as a tiebreaker — since `id` is a UUIDv7 with sub-millisecond precision, this guarantees deterministic latest-wins semantics. All four query variants (episode/inference × cumulative/windowed) are fixed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| target_id, | ||
| value, | ||
| ROW_NUMBER() OVER (PARTITION BY target_id ORDER BY timestamp DESC) as rn | ||
| ROW_NUMBER() OVER (PARTITION BY target_id ORDER BY timestamp DESC, id DESC) as rn |
Member
There was a problem hiding this comment.
I haven't doubled checked here but FYI:
ClickHouse stores UUIDs in Big-endian order. This means that UUIDv7s are not stored chronologically. If you ever want to iterate in order you need to use toUInt128 as the pkey
https://www.notion.so/tensorzerodotcom/ClickHouse-1bc7520bbad380cc8ce6cc3aa02b63cd?source=copy_link
Member
Author
There was a problem hiding this comment.
Good catch — updated all 4 window functions to use toUInt128(id) DESC instead of raw id DESC, consistent with how the rest of this file already orders UUIDv7s.
…ions ClickHouse stores UUIDs in big-endian byte order, which does not preserve chronological ordering for UUIDv7. Use toUInt128(id) as the tiebreaker in ROW_NUMBER() window functions, consistent with the rest of the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
4 participants
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
ROW_NUMBER()window functions for feedback deduplication usedORDER BY timestamp DESC, buttimestampis second-precision (materialized from UUIDv7). When multiple feedback records for the same target land in the same second, the ordering is undefined — causing flaky test failures.id DESCas a tiebreaker to all 4 query variants (episode/inference × cumulative/windowed). Sinceidis a UUIDv7 with sub-millisecond precision, this guarantees deterministic latest-wins semantics.Test plan
test_get_variant_performances_distinct_on_semantics_clickhouseshould now pass deterministically🤖 Generated with Claude Code