support all types

Signed-off-by: Richard Chien <[email protected]>
risingwavelabs · Jan 18, 2024 · 11fef80 · 11fef80
1 parent c5325f7
commit 11fef80
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 34 deletions.
diff --git a/src/common/src/types/interval.rs b/src/common/src/types/interval.rs
@@ -384,6 +384,11 @@ impl Interval {
         self > &Self::from_month_day_usec(0, 0, 0)
     }
 
+    /// Checks if all fields of [`Interval`] are all non-negative.
+    pub fn is_never_negative(&self) -> bool {
+        self.months >= 0 && self.days >= 0 && self.usecs >= 0
+    }
+
     /// Truncate the interval to the precision of milliseconds.
     ///
     /// # Example

diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs
@@ -295,29 +295,32 @@ impl From<DataTypeName> for PbTypeName {
 
 /// Convenient macros to generate match arms for [`DataType`](crate::types::DataType).
 pub mod data_types {
-    /// Numeric [`DataType`](crate::types::DataType)s.
+    /// Numeric [`DataType`](crate::types::DataType)s supported to be `offset` of `RANGE` frame.
     #[macro_export]
-    macro_rules! numeric {
+    macro_rules! range_frame_numeric {
         () => {
             DataType::Int16
                 | DataType::Int32
                 | DataType::Int64
-                | DataType::Serial
                 | DataType::Float32
                 | DataType::Float64
                 | DataType::Decimal
         };
     }
-    pub use numeric;
+    pub use range_frame_numeric;
 
-    /// Integer [`DataType`](crate::types::DataType)s.
+    /// Integer [`DataType`](crate::types::DataType)s supported to be `offset` of `RANGE` frame.
     #[macro_export]
-    macro_rules! integer {
+    macro_rules! range_frame_datetime {
         () => {
-            DataType::Int16 | DataType::Int32 | DataType::Int64
+            DataType::Date
+                | DataType::Time
+                | DataType::Timestamp
+                | DataType::Timestamptz
+                | DataType::Interval
         };
     }
-    pub use integer;
+    pub use range_frame_datetime;
 }
 
 impl DataType {
@@ -373,7 +376,16 @@ impl DataType {
     }
 
     pub fn is_numeric(&self) -> bool {
-        matches!(self, data_types::numeric!())
+        matches!(
+            self,
+            DataType::Int16
+                | DataType::Int32
+                | DataType::Int64
+                | DataType::Serial
+                | DataType::Float32
+                | DataType::Float64
+                | DataType::Decimal
+        )
     }
 
     pub fn is_scalar(&self) -> bool {
@@ -389,7 +401,7 @@ impl DataType {
     }
 
     pub fn is_int(&self) -> bool {
-        matches!(self, data_types::integer!())
+        matches!(self, DataType::Int16 | DataType::Int32 | DataType::Int64)
     }
 
     /// Returns the output type of time window function on a given input type.

diff --git a/src/expr/core/src/window_function/call.rs b/src/expr/core/src/window_function/call.rs
@@ -17,7 +17,7 @@ use std::fmt::Display;
 use enum_as_inner::EnumAsInner;
 use parse_display::Display;
 use risingwave_common::bail;
-use risingwave_common::types::{DataType, ScalarImpl, ScalarRefImpl, ToText};
+use risingwave_common::types::{DataType, IsNegative, ScalarImpl, ScalarRefImpl, ToText};
 use risingwave_pb::expr::window_frame::{PbBound, PbExclusion};
 use risingwave_pb::expr::{PbWindowFrame, PbWindowFunction};
 use FrameBound::{CurrentRow, Following, Preceding, UnboundedFollowing, UnboundedPreceding};
@@ -198,25 +198,33 @@ impl Display for RangeFrameBounds {
 
 impl FrameBoundsImpl for RangeFrameBounds {
     fn validate(&self) -> Result<()> {
+        fn validate_non_negative(val: impl IsNegative + Display) -> Result<()> {
+            if val.is_negative() {
+                bail!(
+                    "frame bound offset should be non-negative, but {} is given",
+                    val
+                );
+            }
+            Ok(())
+        }
+
         FrameBound::validate_bounds(&self.start, &self.end, |offset| {
             match offset.as_scalar_ref_impl() {
-                // TODO(): use decl macro to merge with the following
-                ScalarRefImpl::Int16(val) => {
-                    if val < 0 {
-                        bail!("frame bound offset should be non-negative, but {} is given", val);
-                    }
-                }
-                ScalarRefImpl::Int32(val) => {
-                    if val < 0 {
-                        bail!("frame bound offset should be non-negative, but {} is given", val);
-                    }
-                }
-                ScalarRefImpl::Int64(val) => {
-                    if val < 0 {
-                        bail!("frame bound offset should be non-negative, but {} is given", val);
+                // TODO(rc): use decl macro?
+                ScalarRefImpl::Int16(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Int32(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Int64(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Float32(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Float64(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Decimal(val) => validate_non_negative(val)?,
+                ScalarRefImpl::Interval(val) => {
+                    if !val.is_never_negative() {
+                        bail!(
+                            "for frame bound offset of type `interval`, each field should be non-negative, but {} is given",
+                            val
+                        );
                     }
-                }
-                // TODO(): datetime types
+                },
                 _ => unreachable!("other order column data types are not supported and should be banned in frontend"),
             }
             Ok(())

diff --git a/src/frontend/src/binder/expr/function.rs b/src/frontend/src/binder/expr/function.rs
@@ -697,18 +697,14 @@ impl Binder {
                     let offset_data_type = match order_data_type {
                         // for numeric ordering columns, `offset` should be the same type
                         // NOTE: actually in PG it can be a larger type, but we don't support this here
-                        t @ data_types::numeric!() => t,
+                        t @ data_types::range_frame_numeric!() => t,
                         // for datetime ordering columns, `offset` should be interval
-                        DataType::Date
-                        | DataType::Time
-                        | DataType::Timestamp
-                        | DataType::Timestamptz
-                        | DataType::Interval => DataType::Interval,
+                        data_types::range_frame_datetime!() => DataType::Interval,
                         // other types are not supported
                         t => {
                             return Err(ErrorCode::NotSupported(
                                 format!(
-                                    "`RANGE` frame with offset of `{}` type is not supported",
+                                    "`RANGE` frame with offset of type `{}` is not supported",
                                     t
                                 ),
                                 "Please re-consider the `ORDER BY` column".to_string(),