Skip to content

Commit 9a8b9a5

Browse files
g-talbotclaude
andcommitted
fix: reject duplicate sorted_series column instead of silently skipping
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent cb1b4d2 commit 9a8b9a5

File tree

2 files changed

+16
-6
lines changed

2 files changed

+16
-6
lines changed

quickwit/quickwit-parquet-engine/src/sorted_series/mod.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,13 +102,20 @@ pub fn compute_sorted_series_column(
102102

103103
/// Append the sorted_series column to a [`RecordBatch`].
104104
///
105-
/// If the column already exists, returns the batch unchanged.
105+
/// # Errors
106+
///
107+
/// Returns an error if the column already exists (indicates a bug in
108+
/// the caller), if the sort fields string cannot be parsed, or if
109+
/// storekey encoding fails.
106110
pub fn append_sorted_series_column(
107111
sort_fields_str: &str,
108112
batch: &RecordBatch,
109113
) -> Result<RecordBatch> {
110114
if batch.schema().index_of(SORTED_SERIES_COLUMN).is_ok() {
111-
return Ok(batch.clone());
115+
anyhow::bail!(
116+
"batch already contains a '{}' column — double computation is a bug",
117+
SORTED_SERIES_COLUMN
118+
);
112119
}
113120

114121
let sorted_series = compute_sorted_series_column(sort_fields_str, batch)?;

quickwit/quickwit-parquet-engine/src/sorted_series/tests.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,14 +395,17 @@ fn test_append_sorted_series_column() {
395395
}
396396

397397
#[test]
398-
fn test_append_sorted_series_idempotent() {
398+
fn test_append_sorted_series_rejects_duplicate() {
399399
let batch = build_test_batch(&[TestRow::new("cpu.usage", Some("api"), None, 100)]);
400400

401401
let first = append_sorted_series_column(METRICS_SORT_FIELDS, &batch).unwrap();
402-
let second = append_sorted_series_column(METRICS_SORT_FIELDS, &first).unwrap();
402+
let err = append_sorted_series_column(METRICS_SORT_FIELDS, &first).unwrap_err();
403403

404-
// Second call should be a no-op.
405-
assert_eq!(first.num_columns(), second.num_columns());
404+
assert!(
405+
err.to_string().contains("already contains"),
406+
"expected duplicate-column error, got: {}",
407+
err
408+
);
406409
}
407410

408411
// -----------------------------------------------------------------------

0 commit comments

Comments
 (0)