-
-
Notifications
You must be signed in to change notification settings - Fork 60
Redo: ref(outcomes): fix partitioning for daily table #7396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This PR has a migration; here is the generated SQL for -- start migrations
-- forward migration outcomes : 0010_outcomes_daily_fixed_partitioning
Local op: CREATE TABLE IF NOT EXISTS outcomes_daily_local_v2 (org_id UInt64, project_id UInt64, key_id UInt64, timestamp DateTime, outcome UInt8, reason LowCardinality(String), category UInt8, quantity UInt64, times_seen UInt64) ENGINE ReplicatedSummingMergeTree('/clickhouse/tables/outcomes/{shard}/default/outcomes_daily_local_v2', '{replica}') ORDER BY (org_id, project_id, key_id, outcome, reason, timestamp, category) PARTITION BY (toStartOfMonth(timestamp)) TTL timestamp + toIntervalMonth(13);
Local op: CREATE MATERIALIZED VIEW IF NOT EXISTS outcomes_mv_daily_local_v2 TO outcomes_daily_local_v2 (org_id UInt64, project_id UInt64, key_id UInt64, timestamp DateTime, outcome UInt8, reason String, category UInt8, quantity UInt64, times_seen UInt64) AS
SELECT
org_id,
project_id,
ifNull(key_id, 0) AS key_id,
toStartOfDay(timestamp) AS timestamp,
outcome,
ifNull(reason, 'none') AS reason,
category,
count() AS times_seen,
sum(quantity) AS quantity
FROM outcomes_raw_local
GROUP BY org_id, project_id, key_id, timestamp, outcome, reason, category
;
Distributed op: CREATE TABLE IF NOT EXISTS outcomes_daily_dist_v2 (org_id UInt64, project_id UInt64, key_id UInt64, timestamp DateTime, outcome UInt8, reason LowCardinality(String), category UInt8, quantity UInt64, times_seen UInt64) ENGINE Distributed(`cluster_one_sh`, default, outcomes_daily_local_v2, org_id);
Distributed op: DROP TABLE IF EXISTS outcomes_daily_dist SYNC;
Local op: DROP TABLE IF EXISTS outcomes_mv_daily_local SYNC;
Local op: DROP TABLE IF EXISTS outcomes_daily_local SYNC;
-- end forward migration outcomes : 0010_outcomes_daily_fixed_partitioning
-- backward migration outcomes : 0010_outcomes_daily_fixed_partitioning
Distributed op: DROP TABLE IF EXISTS outcomes_daily_dist_v2 SYNC;
Local op: DROP TABLE IF EXISTS outcomes_mv_daily_local_v2 SYNC;
Local op: DROP TABLE IF EXISTS outcomes_daily_local_v2 SYNC;
-- end backward migration outcomes : 0010_outcomes_daily_fixed_partitioning |
| #[serde(default)] | ||
| group_first_seen: StringToIntDatetime64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: When the group_first_seen field is missing from an error event, the Rust processor incorrectly defaults it to the epoch timestamp (0) instead of a null value.
-
Description: The
ErrorMessagestruct in the Rust errors processor uses#[serde(default)]for thegroup_first_seenfield. Because the underlyingStringToIntDatetime64type implementsDefaultby returning0, any incoming error message from Kafka without this field will havegroup_first_seenset to the Unix epoch timestamp. This is inconsistent with the Python processor, which correctly usesNonefor missing values, and the database schema, which allowsNULL. This incorrect default causes affected issue groups to appear as the oldest possible in any sorting functionality, impacting issue prioritization and display order. -
Suggested fix: Modify the deserialization logic for
group_first_seenin the Rust processor. Instead of#[serde(default)]onStringToIntDatetime64, consider wrapping the type in anOption, likeOption<StringToIntDatetime64>. This will deserialize a missing field toNone, which can then be correctly handled and inserted asNULLinto the database, aligning its behavior with the Python processor.
severity: 0.65, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
53f7fcd to
792ec9d
Compare
|
Redo of #7387