diff --git a/src/mito2/src/engine/alter_test.rs b/src/mito2/src/engine/alter_test.rs index 17f57f2ca0c..36602ffce0f 100644 --- a/src/mito2/src/engine/alter_test.rs +++ b/src/mito2/src/engine/alter_test.rs @@ -499,7 +499,7 @@ async fn test_alter_time_index_column_type_with_format(flat_format: bool) { } #[tokio::test] -async fn test_alter_time_index_unit_overflow_rejected() { +async fn test_alter_time_index_unit_overflow_logged() { common_telemetry::init_default_ut_logging(); let mut env = TestEnv::new().await; @@ -543,23 +543,7 @@ async fn test_alter_time_index_unit_overflow_rejected() { .await; flush_region(&engine, region_id, None).await; - // Widening to nanoseconds would overflow this value and is rejected. - let request = RegionAlterRequest { - kind: AlterKind::ModifyColumnTypes { - columns: vec![ModifyColumnType { - column_name: "ts".to_string(), - target_type: ConcreteDataType::timestamp_nanosecond_datatype(), - }], - }, - }; - let err = engine - .handle_request(region_id, RegionRequest::Alter(request)) - .await - .unwrap_err(); - let chain = full_error_chain(&err); - assert!(chain.contains("overflows the target unit"), "{chain}"); - - // Widening to microseconds is fine: the value fits the target unit. + // Widening to microseconds fits the value. let request = RegionAlterRequest { kind: AlterKind::ModifyColumnTypes { columns: vec![ModifyColumnType { @@ -575,6 +559,25 @@ async fn test_alter_time_index_unit_overflow_rejected() { assert_eq!(vec![32_503_680_000_000_000], { scan_time_index_values(&engine, region_id).await }); + + // Widening to nanoseconds would overflow the value: logged, not rejected. + let request = RegionAlterRequest { + kind: AlterKind::ModifyColumnTypes { + columns: vec![ModifyColumnType { + column_name: "ts".to_string(), + target_type: ConcreteDataType::timestamp_nanosecond_datatype(), + }], + }, + }; + engine + .handle_request(region_id, RegionRequest::Alter(request)) + .await + .unwrap(); + let metadata = engine.get_metadata(region_id).await.unwrap(); + assert_eq!( + ConcreteDataType::timestamp_nanosecond_datatype(), + metadata.time_index_column().column_schema.data_type + ); } /// Scans the region and returns `(ts, field_0)` pairs sorted on ts, so tests diff --git a/src/mito2/src/worker/handle_alter.rs b/src/mito2/src/worker/handle_alter.rs index a531f01bcb0..71fded710e4 100644 --- a/src/mito2/src/worker/handle_alter.rs +++ b/src/mito2/src/worker/handle_alter.rs @@ -162,10 +162,7 @@ impl RegionWorkerLoop { "Try to alter region {}, version.metadata: {:?}, version.options: {:?}, request: {:?}", region_id, version.metadata, version.options, request, ); - if let Err(e) = check_time_index_widening_overflow(region.region_id, &version, &request) { - sender.send(Err(e).context(InvalidRegionRequestSnafu)); - return; - } + log_time_index_widening_overflow(region.region_id, &version, &request); self.handle_alter_region_with_empty_memtable(region, version, request, new_options, sender); } @@ -462,17 +459,17 @@ fn log_option_update( ); } -/// Rejects a time index unit widening whose rescaled values would overflow -/// `i64` in existing data. Memtables are flushed before an alter, so each -/// SST's time range bounds its values; Arrow would cast overflowing values -/// to NULL, which a NOT NULL time index cannot hold. -fn check_time_index_widening_overflow( +/// Logs a time index unit widening whose rescaled values would overflow the +/// target unit's `i64` range in existing data. Memtables are flushed before +/// an alter, so each SST's time range bounds its values; overflowing values +/// cast to NULL on read, which is deemed acceptable. +fn log_time_index_widening_overflow( region_id: RegionId, version: &VersionRef, request: &RegionAlterRequest, -) -> store_api::metadata::Result<()> { +) { let AlterKind::ModifyColumnTypes { columns } = &request.kind else { - return Ok(()); + return; }; let time_index = version.metadata.time_index_column(); for column in columns { @@ -489,27 +486,22 @@ fn check_time_index_widening_overflow( let (start, end) = file.time_range(); if start.convert_to(target_unit).is_none() || end.convert_to(target_unit).is_none() { - let err = format!( - "cannot widen time index column '{}' to {:?}: data spans \ - [{}, {}] which overflows the target unit's i64 range", + error!( + "Time index widening for region {} overflows file {}: widening column \ + '{}' to {:?}, but data spans [{}, {}] beyond the target unit's i64 \ + range; overflowing values read back as NULL", + region_id, + file.file_id(), column.column_name, target_unit, start.to_iso8601_string(), end.to_iso8601_string(), ); - error!( - "Rejecting time index widening for region {}, file {}: {}", - region_id, - file.file_id(), - err - ); - return store_api::metadata::InvalidRegionRequestSnafu { region_id, err } - .fail(); + return; } } } } - Ok(()) } /// Used to determine whether we can build index directly after schema change. diff --git a/tests/cases/standalone/common/alter/change_col_type.result b/tests/cases/standalone/common/alter/change_col_type.result index 94b10671435..9cdfa1c3994 100644 --- a/tests/cases/standalone/common/alter/change_col_type.result +++ b/tests/cases/standalone/common/alter/change_col_type.result @@ -352,22 +352,7 @@ INSERT INTO ts_overflow VALUES ("a", "3000-01-01 00:00:00"); Affected Rows: 1 --- widening to nanoseconds is rejected by region validation: data overflows the target unit --- SQLNESS REPLACE region_id:\s\d+\(\d+,\s\d+\) region_id: REDACTED -ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_NS; - -Error: 1004(InvalidArguments), Invalid region request, region_id: REDACTED, err: cannot widen time index column 'ts' to Nanosecond: data spans [3000-01-01 00:00:00+0000, 3000-01-01 00:00:00+0000] which overflows the target unit's i64 range - -DESCRIBE ts_overflow; - -+--------+----------------------+-----+------+---------+---------------+ -| Column | Type | Key | Null | Default | Semantic Type | -+--------+----------------------+-----+------+---------+---------------+ -| host | String | | YES | | FIELD | -| ts | TimestampMillisecond | PRI | NO | | TIMESTAMP | -+--------+----------------------+-----+------+---------+---------------+ - --- widening to microseconds still works +-- widening to microseconds fits the data ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_US; Affected Rows: 0 @@ -381,6 +366,20 @@ DESCRIBE ts_overflow; | ts | TimestampMicrosecond | PRI | NO | | TIMESTAMP | +--------+----------------------+-----+------+---------+---------------+ +-- widening to nanoseconds would overflow the data: allowed, only logged +ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_NS; + +Affected Rows: 0 + +DESCRIBE ts_overflow; + ++--------+---------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++--------+---------------------+-----+------+---------+---------------+ +| host | String | | YES | | FIELD | +| ts | TimestampNanosecond | PRI | NO | | TIMESTAMP | ++--------+---------------------+-----+------+---------+---------------+ + DROP TABLE ts_overflow; Affected Rows: 0 diff --git a/tests/cases/standalone/common/alter/change_col_type.sql b/tests/cases/standalone/common/alter/change_col_type.sql index 3875ad29cec..3356112cd0b 100644 --- a/tests/cases/standalone/common/alter/change_col_type.sql +++ b/tests/cases/standalone/common/alter/change_col_type.sql @@ -125,15 +125,14 @@ CREATE TABLE ts_overflow (host STRING, ts TIMESTAMP TIME INDEX); -- 3000-01-01 fits milliseconds but overflows nanoseconds (max ~2262-04-11). INSERT INTO ts_overflow VALUES ("a", "3000-01-01 00:00:00"); --- widening to nanoseconds is rejected by region validation: data overflows the target unit --- SQLNESS REPLACE region_id:\s\d+\(\d+,\s\d+\) region_id: REDACTED -ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_NS; - -DESCRIBE ts_overflow; - --- widening to microseconds still works +-- widening to microseconds fits the data ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_US; DESCRIBE ts_overflow; +-- widening to nanoseconds would overflow the data: allowed, only logged +ALTER TABLE ts_overflow MODIFY COLUMN ts TIMESTAMP_NS; + +DESCRIBE ts_overflow; + DROP TABLE ts_overflow;