Skip to content

Commit 0761d11

Browse files
g-talbotclaude
andcommitted
fix(31): correct postgres types for window_duration_secs and zonemap_regexes
Gap 1: Change window_duration_secs from i32 to Option<i32> in both PgMetricsSplit and InsertableMetricsSplit. Pre-Phase-31 splits now correctly map 0 → NULL in PostgreSQL, enabling Phase 32 compaction queries to use `WHERE window_duration_secs IS NOT NULL` instead of the fragile `WHERE window_duration_secs > 0`. Gap 2: Change zonemap_regexes from String to serde_json::Value in both structs. This maps directly to JSONB in sqlx, avoiding ambiguity when PostgreSQL JSONB operators are used in Phase 34/35 zonemap pruning. Gap 3: Add two missing tests: - test_insertable_from_metadata_with_compaction_fields: verifies all 6 compaction fields round-trip through InsertableMetricsSplit - test_insertable_from_metadata_pre_phase31_defaults: verifies pre-Phase-31 metadata produces window_duration_secs: None, zonemap_regexes: json!({}) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1786818 commit 0761d11

File tree

2 files changed

+71
-21
lines changed

2 files changed

+71
-21
lines changed

quickwit/quickwit-metastore/src/metastore/postgres/metastore.rs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1844,15 +1844,11 @@ impl MetastoreService for PostgresqlMetastore {
18441844
size_bytes_list.push(insertable.size_bytes);
18451845
split_metadata_jsons.push(insertable.split_metadata_json);
18461846
window_starts.push(insertable.window_start);
1847-
window_duration_secs_list.push(if insertable.window_duration_secs == 0 {
1848-
None
1849-
} else {
1850-
Some(insertable.window_duration_secs)
1851-
});
1847+
window_duration_secs_list.push(insertable.window_duration_secs);
18521848
sort_fields_list.push(insertable.sort_fields);
18531849
num_merge_ops_list.push(insertable.num_merge_ops);
18541850
row_keys_list.push(insertable.row_keys);
1855-
zonemap_regexes_json_list.push(insertable.zonemap_regexes);
1851+
zonemap_regexes_json_list.push(insertable.zonemap_regexes.to_string());
18561852
}
18571853

18581854
info!(
@@ -2384,11 +2380,11 @@ impl MetastoreService for PostgresqlMetastore {
23842380
// to_metadata() — the SQL columns are only used for
23852381
// filtering and SS-5 consistency checks.
23862382
window_start: None,
2387-
window_duration_secs: 0,
2383+
window_duration_secs: None,
23882384
sort_fields: String::new(),
23892385
num_merge_ops: 0,
23902386
row_keys: None,
2391-
zonemap_regexes: "{}".to_string(),
2387+
zonemap_regexes: serde_json::json!({}),
23922388
};
23932389

23942390
let state = pg_split.split_state().unwrap_or(MetricsSplitState::Staged);

quickwit/quickwit-parquet-engine/src/split/postgres.rs

Lines changed: 67 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,11 @@ pub struct PgMetricsSplit {
7272
pub split_metadata_json: String,
7373
pub update_timestamp: i64,
7474
pub window_start: Option<i64>,
75-
pub window_duration_secs: i32,
75+
pub window_duration_secs: Option<i32>,
7676
pub sort_fields: String,
7777
pub num_merge_ops: i32,
7878
pub row_keys: Option<Vec<u8>>,
79-
pub zonemap_regexes: String,
79+
pub zonemap_regexes: serde_json::Value,
8080
}
8181

8282
/// Insertable row for metrics_splits table.
@@ -99,11 +99,11 @@ pub struct InsertableMetricsSplit {
9999
pub size_bytes: i64,
100100
pub split_metadata_json: String,
101101
pub window_start: Option<i64>,
102-
pub window_duration_secs: i32,
102+
pub window_duration_secs: Option<i32>,
103103
pub sort_fields: String,
104104
pub num_merge_ops: i32,
105105
pub row_keys: Option<Vec<u8>>,
106-
pub zonemap_regexes: String,
106+
pub zonemap_regexes: serde_json::Value,
107107
}
108108

109109
impl InsertableMetricsSplit {
@@ -114,12 +114,6 @@ impl InsertableMetricsSplit {
114114
) -> Result<Self, serde_json::Error> {
115115
let split_metadata_json = serde_json::to_string(metadata)?;
116116

117-
let zonemap_regexes_json = if metadata.zonemap_regexes.is_empty() {
118-
"{}".to_string()
119-
} else {
120-
serde_json::to_string(&metadata.zonemap_regexes)?
121-
};
122-
123117
Ok(Self {
124118
split_id: metadata.split_id.as_str().to_string(),
125119
split_state: state.as_str().to_string(),
@@ -137,11 +131,16 @@ impl InsertableMetricsSplit {
137131
size_bytes: metadata.size_bytes as i64,
138132
split_metadata_json,
139133
window_start: metadata.window_start,
140-
window_duration_secs: metadata.window_duration_secs as i32,
134+
window_duration_secs: if metadata.window_duration_secs > 0 {
135+
Some(metadata.window_duration_secs as i32)
136+
} else {
137+
None
138+
},
141139
sort_fields: metadata.sort_fields.clone(),
142140
num_merge_ops: metadata.num_merge_ops as i32,
143141
row_keys: metadata.row_keys_proto.clone(),
144-
zonemap_regexes: zonemap_regexes_json,
142+
zonemap_regexes: serde_json::to_value(&metadata.zonemap_regexes)
143+
.unwrap_or_else(|_| serde_json::json!({})),
145144
})
146145
}
147146
}
@@ -165,7 +164,7 @@ impl PgMetricsSplit {
165164
debug_assert_eq!(metadata.window_start, self.window_start);
166165
debug_assert_eq!(
167166
metadata.window_duration_secs,
168-
self.window_duration_secs as u32
167+
self.window_duration_secs.unwrap_or(0) as u32
169168
);
170169
debug_assert_eq!(metadata.sort_fields, self.sort_fields);
171170
debug_assert_eq!(metadata.num_merge_ops, self.num_merge_ops as u32);
@@ -257,6 +256,61 @@ mod tests {
257256
assert_eq!(insertable.size_bytes, 1024 * 1024);
258257
}
259258

259+
#[test]
260+
fn test_insertable_from_metadata_with_compaction_fields() {
261+
let metadata = MetricsSplitMetadata::builder()
262+
.split_id(SplitId::new("compaction-test"))
263+
.index_uid("test-index:00000000000000000000000000")
264+
.time_range(TimeRange::new(1000, 2000))
265+
.num_rows(100)
266+
.size_bytes(500)
267+
.window_start_secs(1700000000)
268+
.window_duration_secs(3600)
269+
.sort_fields("metric_name|host|timestamp/V2")
270+
.num_merge_ops(2)
271+
.row_keys_proto(vec![0x08, 0x01])
272+
.add_zonemap_regex("metric_name", "cpu\\..*")
273+
.build();
274+
275+
let insertable =
276+
InsertableMetricsSplit::from_metadata(&metadata, MetricsSplitState::Published)
277+
.expect("conversion should succeed");
278+
279+
assert_eq!(insertable.window_start, Some(1700000000));
280+
assert_eq!(insertable.window_duration_secs, Some(3600));
281+
assert_eq!(insertable.sort_fields, "metric_name|host|timestamp/V2");
282+
assert_eq!(insertable.num_merge_ops, 2);
283+
assert_eq!(insertable.row_keys, Some(vec![0x08, 0x01]));
284+
assert!(insertable.zonemap_regexes.is_object());
285+
assert_eq!(
286+
insertable.zonemap_regexes["metric_name"],
287+
serde_json::json!("cpu\\..*")
288+
);
289+
}
290+
291+
#[test]
292+
fn test_insertable_from_metadata_pre_phase31_defaults() {
293+
let metadata = MetricsSplitMetadata::builder()
294+
.split_id(SplitId::new("pre-phase31"))
295+
.index_uid("test-index:00000000000000000000000000")
296+
.time_range(TimeRange::new(1000, 2000))
297+
.build();
298+
299+
let insertable =
300+
InsertableMetricsSplit::from_metadata(&metadata, MetricsSplitState::Staged)
301+
.expect("conversion should succeed");
302+
303+
assert!(insertable.window_start.is_none());
304+
assert!(
305+
insertable.window_duration_secs.is_none(),
306+
"pre-Phase-31 splits should have NULL window_duration_secs"
307+
);
308+
assert_eq!(insertable.sort_fields, "");
309+
assert_eq!(insertable.num_merge_ops, 0);
310+
assert!(insertable.row_keys.is_none());
311+
assert_eq!(insertable.zonemap_regexes, serde_json::json!({}));
312+
}
313+
260314
#[test]
261315
fn test_pg_split_to_metadata_roundtrip() {
262316
let original = MetricsSplitMetadata::builder()

0 commit comments

Comments
 (0)