From 85c013661903bee3f0dc8b972a45992e43ae54f8 Mon Sep 17 00:00:00 2001 From: jeremyhi Date: Thu, 10 Jul 2025 09:53:00 +0800 Subject: [PATCH] fix: greptime timestamp display null (#6469) * feat: is_overflow method * feat: check ts overflow --- src/common/sql/src/convert.rs | 100 ++++++++++++++++++++++++++++++- src/common/time/src/timestamp.rs | 11 ++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/src/common/sql/src/convert.rs b/src/common/sql/src/convert.rs index a21d6c9b32..7f625ee887 100644 --- a/src/common/sql/src/convert.rs +++ b/src/common/sql/src/convert.rs @@ -56,8 +56,18 @@ macro_rules! parse_number_to_value { }, )+ ConcreteDataType::Timestamp(t) => { - let n = parse_sql_number::($n)?; - Ok(Value::Timestamp(Timestamp::new(n, t.unit()))) + let n = parse_sql_number::($n)?; + let timestamp = Timestamp::new(n, t.unit()); + + // Check if the value is within the valid range for the target unit + if Timestamp::is_overflow(n, t.unit()) { + return TimestampOverflowSnafu { + timestamp, + target_unit: t.unit(), + }.fail(); + } + + Ok(Value::Timestamp(timestamp)) }, // TODO(QuenKar): This could need to be optimized // if this from_str function is slow, @@ -362,6 +372,7 @@ pub(crate) fn parse_hex_string(s: &str) -> Result { mod test { use common_base::bytes::Bytes; use common_time::timestamp::TimeUnit; + use datatypes::types::TimestampType; use datatypes::value::OrderedFloat; use super::*; @@ -1081,4 +1092,89 @@ mod test { ); assert!(v.is_ok()); } + + #[test] + fn test_sql_number_to_value_timestamp_strict_typing() { + // Test that values are interpreted according to the target column type + let timestamp_type = TimestampType::Millisecond(datatypes::types::TimestampMillisecondType); + let data_type = ConcreteDataType::Timestamp(timestamp_type); + + // Valid millisecond timestamp + let millisecond_str = "1747814093865"; + let result = sql_number_to_value(&data_type, millisecond_str).unwrap(); + if let Value::Timestamp(ts) = result { + assert_eq!(ts.unit(), TimeUnit::Millisecond); + assert_eq!(ts.value(), 1747814093865); + } else { + panic!("Expected timestamp value"); + } + + // Large value that would overflow when treated as milliseconds should be rejected + let nanosecond_str = "1747814093865000000"; // This is too large for millisecond precision + let result = sql_number_to_value(&data_type, nanosecond_str); + assert!( + result.is_err(), + "Should reject overly large timestamp values" + ); + } + + #[test] + fn test_sql_number_to_value_timestamp_different_units() { + // Test second precision + let second_type = TimestampType::Second(datatypes::types::TimestampSecondType); + let second_data_type = ConcreteDataType::Timestamp(second_type); + + let second_str = "1747814093"; + let result = sql_number_to_value(&second_data_type, second_str).unwrap(); + if let Value::Timestamp(ts) = result { + assert_eq!(ts.unit(), TimeUnit::Second); + assert_eq!(ts.value(), 1747814093); + } else { + panic!("Expected timestamp value"); + } + + // Test nanosecond precision + let nanosecond_type = TimestampType::Nanosecond(datatypes::types::TimestampNanosecondType); + let nanosecond_data_type = ConcreteDataType::Timestamp(nanosecond_type); + + let nanosecond_str = "1747814093865000000"; + let result = sql_number_to_value(&nanosecond_data_type, nanosecond_str).unwrap(); + if let Value::Timestamp(ts) = result { + assert_eq!(ts.unit(), TimeUnit::Nanosecond); + assert_eq!(ts.value(), 1747814093865000000); + } else { + panic!("Expected timestamp value"); + } + } + + #[test] + fn test_timestamp_range_validation() { + // Test that our range checking works correctly + let nanosecond_value = 1747814093865000000i64; // This should be too large for millisecond + + // This should work for nanosecond precision + let nanosecond_type = TimestampType::Nanosecond(datatypes::types::TimestampNanosecondType); + let nanosecond_data_type = ConcreteDataType::Timestamp(nanosecond_type); + let result = sql_number_to_value(&nanosecond_data_type, "1747814093865000000"); + assert!( + result.is_ok(), + "Nanosecond value should be valid for nanosecond column" + ); + + // This should fail for millisecond precision (value too large) + let millisecond_type = + TimestampType::Millisecond(datatypes::types::TimestampMillisecondType); + let millisecond_data_type = ConcreteDataType::Timestamp(millisecond_type); + let result = sql_number_to_value(&millisecond_data_type, "1747814093865000000"); + assert!( + result.is_err(), + "Nanosecond value should be rejected for millisecond column" + ); + + // Verify the ranges work as expected + assert!( + nanosecond_value > Timestamp::MAX_MILLISECOND.value(), + "Test value should exceed millisecond range" + ); + } } diff --git a/src/common/time/src/timestamp.rs b/src/common/time/src/timestamp.rs index 773676df86..4656b24d76 100644 --- a/src/common/time/src/timestamp.rs +++ b/src/common/time/src/timestamp.rs @@ -498,6 +498,17 @@ impl Timestamp { pub const MIN_NANOSECOND: Self = Self::new_nanosecond(i64::MIN); pub const MAX_NANOSECOND: Self = Self::new_nanosecond(i64::MAX); + + /// Checks if a value would overflow for the given time unit. + pub fn is_overflow(value: i64, unit: TimeUnit) -> bool { + let (min_val, max_val) = match unit { + TimeUnit::Second => (Self::MIN_SECOND.value(), Self::MAX_SECOND.value()), + TimeUnit::Millisecond => (Self::MIN_MILLISECOND.value(), Self::MAX_MILLISECOND.value()), + TimeUnit::Microsecond => (Self::MIN_MICROSECOND.value(), Self::MAX_MICROSECOND.value()), + TimeUnit::Nanosecond => (Self::MIN_NANOSECOND.value(), Self::MAX_NANOSECOND.value()), + }; + value < min_val || value > max_val + } } /// Converts the naive datetime (which has no specific timezone) to a