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.

Sign up for GitHub

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

Jump to bottom

refactor: Refactor error in DataType #36

Merged
merged 6 commits into from
Aug 4, 2024
Merged

refactor: Refactor error in DataType #36

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7ce03a
refactor: Refactor error in DataType
XuQianJin-Stars Aug 3, 2024
715ffb3
refactor: Refactor error in DataType
XuQianJin-Stars Aug 3, 2024
2167dbd
refactor: Refactor error in DataType
XuQianJin-Stars Aug 3, 2024
3edf01e
refactor: Refactor error in DataType
XuQianJin-Stars Aug 3, 2024
d10fc4c
refactor: Refactor error in DataType
XuQianJin-Stars Aug 4, 2024
1791515
refactor: Refactor error in DataType
XuQianJin-Stars Aug 4, 2024
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
9 changes: 9 additions & 0 deletions crates/paimon/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,12 @@ pub enum Error {
source: opendal::Error,
},
}

#[derive(Debug, Snafu)]
pub enum DataTypeError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, we usually expose only one error in each crate to keep them user-friendly. I will suggest to add DataTypeInvalid as a new enum in Error directly. In this way, we can use paimon::Error everywhere.

#[snafu(
visibility(pub(crate)),
display("Paimon data type invalid for {}", message)
)]
DataTypeInvalid { message: String },
}
131 changes: 81 additions & 50 deletions crates/paimon/src/spec/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

use crate::error::Error;
use crate::error::*;
use crate::spec::DataField;
use bitflags::bitflags;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -225,13 +225,16 @@ impl BinaryType {

pub const DEFAULT_LENGTH: usize = 1;

pub fn new(length: usize) -> Result<Self, &'static str> {
pub fn new(length: usize) -> Result<Self, DataTypeError> {
Self::with_nullable(true, length)
}

pub fn with_nullable(nullable: bool, length: usize) -> Result<Self, &'static str> {
pub fn with_nullable(nullable: bool, length: usize) -> Result<Self, DataTypeError> {
if length < Self::MIN_LENGTH {
return Err("Binary string length must be at least 1.");
return DataTypeInvalidSnafu {
message: "Binary string length must be at least 1.".to_string(),
}
.fail();
}
Ok(Self { nullable, length })
}
Expand Down Expand Up @@ -320,13 +323,16 @@ impl CharType {

pub const MAX_LENGTH: usize = 255;

pub fn new(length: usize) -> Result<Self, &'static str> {
pub fn new(length: usize) -> Result<Self, DataTypeError> {
Self::with_nullable(true, length)
}

pub fn with_nullable(nullable: bool, length: usize) -> Result<Self, &'static str> {
pub fn with_nullable(nullable: bool, length: usize) -> Result<Self, DataTypeError> {
if !(Self::MIN_LENGTH..=Self::MAX_LENGTH).contains(&length) {
return Err("Character string length must be between 1 and 255 (both inclusive).");
return DataTypeInvalidSnafu {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this by ensure!(length < Self::MIN_LENGTH, DataTypeInvalidSnafu {message: "Binary string length must be at least 1."}); ?

The same as below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this by ensure!(length < Self::MIN_LENGTH, DataTypeInvalidSnafu {message: "Binary string length must be at least 1."}); ?

The same as below

This simplifies things a bit, but the readability of the code is reduced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, make sense

message: "Char string length must be between 1 and 255.".to_string(),
}
.fail();
}
Ok(CharType { nullable, length })
}
Expand Down Expand Up @@ -420,25 +426,35 @@ impl DecimalType {

pub const DEFAULT_SCALE: u32 = 0;

pub fn new(precision: u32, scale: u32) -> Result<Self, String> {
pub fn new(precision: u32, scale: u32) -> Result<Self, DataTypeError> {
Self::with_nullable(true, precision, scale)
}

pub fn with_nullable(nullable: bool, precision: u32, scale: u32) -> Result<Self, String> {
pub fn with_nullable(
nullable: bool,
precision: u32,
scale: u32,
) -> Result<Self, DataTypeError> {
if !(Self::MIN_PRECISION..=Self::MAX_PRECISION).contains(&precision) {
return Err(format!(
"Decimal precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
));
return DataTypeInvalidSnafu {
message: format!(
"Decimal precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
),
}
.fail();
}

if !(Self::MIN_SCALE..=precision).contains(&scale) {
return Err(format!(
"Decimal scale must be between {} and the precision {} (both inclusive).",
Self::MIN_SCALE,
precision
));
return DataTypeInvalidSnafu {
message: format!(
"Decimal scale must be between {} and {} (both inclusive).",
Self::MIN_SCALE,
precision
),
}
.fail();
}

Ok(DecimalType {
Expand Down Expand Up @@ -616,17 +632,20 @@ impl LocalZonedTimestampType {

pub const DEFAULT_PRECISION: u32 = TimestampType::DEFAULT_PRECISION;

pub fn new(precision: u32) -> Result<Self, String> {
pub fn new(precision: u32) -> Result<Self, DataTypeError> {
Self::with_nullable(true, precision)
}

pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, String> {
pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, DataTypeError> {
if !(Self::MIN_PRECISION..=Self::MAX_PRECISION).contains(&precision) {
return Err(format!(
"Timestamp precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
));
return DataTypeInvalidSnafu {
message: format!(
"LocalZonedTimestamp precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
),
}
.fail();
}

Ok(LocalZonedTimestampType {
Expand Down Expand Up @@ -725,17 +744,20 @@ impl TimeType {

pub const DEFAULT_PRECISION: u32 = 0;

pub fn new(precision: u32) -> Result<Self, String> {
pub fn new(precision: u32) -> Result<Self, DataTypeError> {
Self::with_nullable(true, precision)
}

pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, String> {
pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, DataTypeError> {
if !(Self::MIN_PRECISION..=Self::MAX_PRECISION).contains(&precision) {
return Err(format!(
"Time precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
));
return DataTypeInvalidSnafu {
message: format!(
"Time precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
),
}
.fail();
}

Ok(TimeType {
Expand Down Expand Up @@ -787,17 +809,20 @@ impl TimestampType {

pub const DEFAULT_PRECISION: u32 = 6;

pub fn new(precision: u32) -> Result<Self, String> {
pub fn new(precision: u32) -> Result<Self, DataTypeError> {
Self::with_nullable(true, precision)
}

pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, String> {
pub fn with_nullable(nullable: bool, precision: u32) -> Result<Self, DataTypeError> {
if !(Self::MIN_PRECISION..=Self::MAX_PRECISION).contains(&precision) {
return Err(format!(
"Timestamp precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
));
return DataTypeInvalidSnafu {
message: format!(
"Timestamp precision must be between {} and {} (both inclusive).",
Self::MIN_PRECISION,
Self::MAX_PRECISION
),
}
.fail();
}

Ok(TimestampType {
Expand Down Expand Up @@ -892,13 +917,16 @@ impl VarBinaryType {

pub const DEFAULT_LENGTH: u32 = 1;

pub fn new(length: u32) -> Result<Self, String> {
pub fn new(length: u32) -> Result<Self, DataTypeError> {
Self::try_new(true, length)
}

pub fn try_new(nullable: bool, length: u32) -> Result<Self, String> {
pub fn try_new(nullable: bool, length: u32) -> Result<Self, DataTypeError> {
if length < Self::MIN_LENGTH {
return Err("Binary string length must be at least 1.".to_string());
return DataTypeInvalidSnafu {
message: "VarBinary string length must be at least 1.".to_string(),
}
.fail();
}

Ok(VarBinaryType { nullable, length })
Expand Down Expand Up @@ -947,17 +975,20 @@ impl VarCharType {

pub const DEFAULT_LENGTH: u32 = 1;

pub fn new(length: u32) -> Result<Self, String> {
pub fn new(length: u32) -> Result<Self, DataTypeError> {
Self::with_nullable(true, length)
}

pub fn with_nullable(nullable: bool, length: u32) -> Result<Self, String> {
pub fn with_nullable(nullable: bool, length: u32) -> Result<Self, DataTypeError> {
if !(Self::MIN_LENGTH..=Self::MAX_LENGTH).contains(&length) {
return Err(format!(
"Character string length must be between {} and {} (both inclusive).",
Self::MIN_LENGTH,
Self::MAX_LENGTH
));
return DataTypeInvalidSnafu {
message: format!(
"VarChar string length must be between {} and {} (both inclusive).",
Self::MIN_LENGTH,
Self::MAX_LENGTH
),
}
.fail();
}

Ok(VarCharType { nullable, length })
Expand Down
Loading