rust/greptimedb
1
0
Fork 0
mirror of https://github.com/GreptimeTeam/greptimedb.git synced 2026-11-14 23:06:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: log instead of rejecting time index widening overflow

Browse Source 
Overflowing values cast to NULL on read but do not otherwise affect
reads or writes, so the alter is allowed; the region-level check now
only logs (with the offending file) when an SST's time range exceeds
the target unit's i64 range.

Signed-off-by: Ning Sun <sunning@greptime.com>
This commit is contained in:
Ning Sun
2026-09-02 11:27:12 +08:00
parent 1edff05f16
commit e5e7396ffb
4 changed files with 57 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/mito2/src/engine/alter_test.rs
+21 -18
View File
@@ -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
src/mito2/src/worker/handle_alter.rs
+15 -23
View File
@@ -162,10 +162,7 @@ impl<S: LogStore> RegionWorkerLoop<S> {
"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<T: std::fmt::Debug>(
);
}
/// 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.
tests/cases/standalone/common/alter/change_col_type.result
+15 -16
View File
@@ -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
tests/cases/standalone/common/alter/change_col_type.sql
+6 -7
View File
@@ -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;