Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(common): make certain IntervalUnit constructors test-only #8464

Merged
merged 2 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/batch/src/executor/hop_window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ mod tests {
use futures::stream::StreamExt;
use risingwave_common::array::{DataChunk, DataChunkTestExt};
use risingwave_common::catalog::{Field, Schema};
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::DataType;
use risingwave_expr::expr::test_utils::make_hop_window_expression;

Expand Down
1 change: 1 addition & 0 deletions src/common/src/array/arrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ impl From<&arrow_array::StructArray> for StructArray {
#[cfg(test)]
mod tests {
use super::*;
use crate::types::interval::test_utils::IntervalUnitTestExt;
use crate::{array, empty_array};

#[test]
Expand Down
1 change: 1 addition & 0 deletions src/common/src/array/interval_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ mod tests {
use super::IntervalArray;
use crate::array::interval_array::{IntervalArrayBuilder, IntervalUnit};
use crate::array::{Array, ArrayBuilder};
use crate::types::interval::test_utils::IntervalUnitTestExt;

#[test]
fn test_interval_array() {
Expand Down
125 changes: 72 additions & 53 deletions src/common/src/types/interval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,46 +109,6 @@ impl IntervalUnit {
}
}

#[must_use]
pub fn from_ymd(year: i32, month: i32, days: i32) -> Self {
let months = year * 12 + month;
let days = days;
let ms = 0;
IntervalUnit { months, days, ms }
}

#[must_use]
pub fn from_month(months: i32) -> Self {
IntervalUnit {
months,
..Default::default()
}
}

#[must_use]
pub fn from_days(days: i32) -> Self {
Self {
days,
..Default::default()
}
}

#[must_use]
pub fn from_millis(ms: i64) -> Self {
Self {
ms,
..Default::default()
}
}

#[must_use]
pub fn from_minutes(minutes: i64) -> Self {
Self {
ms: 1000 * 60 * minutes,
..Default::default()
}
}

pub fn to_protobuf<T: Write>(self, output: &mut T) -> ArrayResult<usize> {
output.write_i32::<BigEndian>(self.months)?;
output.write_i32::<BigEndian>(self.days)?;
Expand Down Expand Up @@ -435,6 +395,63 @@ impl IntervalUnit {
}
}

/// A separate mod so that `use types::*` or `use interval::*` does not `use IntervalUnitTestExt` by
/// accident.
pub mod test_utils {
use super::*;

/// These constructors may panic when value out of bound. Only use in tests with known input.
pub trait IntervalUnitTestExt {
fn from_ymd(year: i32, month: i32, days: i32) -> Self;
fn from_month(months: i32) -> Self;
fn from_days(days: i32) -> Self;
fn from_millis(ms: i64) -> Self;
fn from_minutes(minutes: i64) -> Self;
}

impl IntervalUnitTestExt for IntervalUnit {
#[must_use]
fn from_ymd(year: i32, month: i32, days: i32) -> Self {
let months = year * 12 + month;
let days = days;
let ms = 0;
IntervalUnit { months, days, ms }
}

#[must_use]
fn from_month(months: i32) -> Self {
IntervalUnit {
months,
..Default::default()
}
}

#[must_use]
fn from_days(days: i32) -> Self {
Self {
days,
..Default::default()
}
}

#[must_use]
fn from_millis(ms: i64) -> Self {
Self {
ms,
..Default::default()
}
}

#[must_use]
fn from_minutes(minutes: i64) -> Self {
Self {
ms: 1000 * 60 * minutes,
..Default::default()
}
}
}
}

/// Wrapper so that `Debug for IntervalUnitDisplay` would use the concise format of `Display for
/// IntervalUnit`.
#[derive(Clone, Copy)]
Expand Down Expand Up @@ -926,21 +943,21 @@ impl IntervalUnit {
(|| match leading_field {
Year => {
let months = num.checked_mul(12)?;
Some(IntervalUnit::from_month(months as i32))
Some(IntervalUnit::new(months as i32, 0, 0))
}
Month => Some(IntervalUnit::from_month(num as i32)),
Day => Some(IntervalUnit::from_days(num as i32)),
Month => Some(IntervalUnit::new(num as i32, 0, 0)),
Day => Some(IntervalUnit::new(0, num as i32, 0)),
Hour => {
let ms = num.checked_mul(3600 * 1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
Minute => {
let ms = num.checked_mul(60 * 1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
Second => {
let ms = num.checked_mul(1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
})()
.ok_or_else(|| ErrorCode::InvalidInputSyntax(format!("Invalid interval {}.", s)).into())
Expand All @@ -963,21 +980,21 @@ impl IntervalUnit {
result = result + (|| match interval_unit {
Year => {
let months = num.checked_mul(12)?;
Some(IntervalUnit::from_month(months as i32))
Some(IntervalUnit::new(months as i32, 0, 0))
}
Month => Some(IntervalUnit::from_month(num as i32)),
Day => Some(IntervalUnit::from_days(num as i32)),
Month => Some(IntervalUnit::new(num as i32, 0, 0)),
Day => Some(IntervalUnit::new(0, num as i32, 0)),
Hour => {
let ms = num.checked_mul(3600 * 1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
Minute => {
let ms = num.checked_mul(60 * 1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
Second => {
let ms = num.checked_mul(1000)?;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
})()
.ok_or_else(|| ErrorCode::InvalidInputSyntax(format!("Invalid interval {}.", s)))?;
Expand All @@ -989,7 +1006,7 @@ impl IntervalUnit {
// If unsatisfied precision is passed as input, we should not return None (Error).
// TODO: IntervalUnit only support millisecond precision so the part smaller than millisecond will be truncated.
let ms = (second.into_inner() * 1000_f64).round() as i64;
Some(IntervalUnit::from_millis(ms))
Some(IntervalUnit::new(0, 0, ms))
}
_ => None,
}
Expand Down Expand Up @@ -1022,6 +1039,8 @@ impl FromStr for IntervalUnit {

#[cfg(test)]
mod tests {
use interval::test_utils::IntervalUnitTestExt;

use super::*;
use crate::types::ordered_float::OrderedFloat;

Expand Down
1 change: 1 addition & 0 deletions src/expr/benches/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

use criterion::{criterion_group, criterion_main, Criterion};
use risingwave_common::array::*;
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{
DataType, DataTypeName, Decimal, IntervalUnit, NaiveDateTimeWrapper, NaiveDateWrapper,
NaiveTimeWrapper, OrderedF32, OrderedF64,
Expand Down
1 change: 1 addition & 0 deletions src/expr/src/expr/expr_binary_nonnull.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ fn boolean_le(l: &BoolArray, r: &BoolArray) -> BoolArray {
mod tests {
use risingwave_common::array::interval_array::IntervalArray;
use risingwave_common::array::*;
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{
Decimal, IntervalUnit, NaiveDateTimeWrapper, NaiveDateWrapper, Scalar,
};
Expand Down
1 change: 1 addition & 0 deletions src/expr/src/expr/expr_literal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ impl<'a> TryFrom<&'a ExprNode> for LiteralExpression {
mod tests {
use risingwave_common::array::{I32Array, StructValue};
use risingwave_common::array_nonnull;
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{Decimal, IntervalUnit, IntoOrdered};
use risingwave_common::util::value_encoding::serialize_datum;
use risingwave_pb::data::data_type::{IntervalType, TypeName};
Expand Down
1 change: 1 addition & 0 deletions src/expr/src/table_function/generate_series.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ pub fn new_generate_series<const STOP_INCLUSIVE: bool>(

#[cfg(test)]
mod tests {
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{DataType, IntervalUnit, NaiveDateTimeWrapper, ScalarImpl};

use super::*;
Expand Down
1 change: 1 addition & 0 deletions src/expr/src/vector_op/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::assert_matches::assert_matches;
use std::str::FromStr;

use chrono::NaiveDateTime;
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{
Decimal, IntervalUnit, NaiveDateTimeWrapper, NaiveDateWrapper, OrderedF32, OrderedF64,
};
Expand Down
1 change: 1 addition & 0 deletions src/frontend/src/binder/expr/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ fn unescape_c_style(s: &str) -> Result<String> {

#[cfg(test)]
mod tests {
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::DataType;
use risingwave_expr::expr::build_from_prost;
use risingwave_sqlparser::ast::Value::Number;
Expand Down
1 change: 1 addition & 0 deletions src/stream/src/executor/hop_window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ mod tests {
use futures::StreamExt;
use risingwave_common::array::stream_chunk::StreamChunkTestExt;
use risingwave_common::catalog::{Field, Schema};
use risingwave_common::types::test_utils::IntervalUnitTestExt;
use risingwave_common::types::{DataType, IntervalUnit};
use risingwave_expr::expr::test_utils::make_hop_window_expression;

Expand Down