Skip to content

Commit 88f3d40

Browse files
Dustin-RayDustin-Ray
authored
refactor: conversion error decimal variant (#26)
# Rationale for this change The ```ConversionError``` error type has grown quite large with the addition of new features and types. This PR categorizes some timestamp and decimal errors into different modules and reduces the size and complexity of ```ConversionError```. # What changes are included in this PR? IntermediateDecimal, Decimal errors are moved to respective modules. Native thiserr ```#from``` is used in place of manual ```From``` conversions. # Are these changes tested? yes
1 parent 55816c3 commit 88f3d40

File tree

7 files changed

+126
-95
lines changed

7 files changed

+126
-95
lines changed

‎crates/proof-of-sql-parser/src/intermediate_decimal.rs

+20-23
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,15 @@
55
//!
66
//! A decimal must have a decimal point. The lexer does not route
77
//! whole integers to this contructor.
8+
use crate::intermediate_decimal::IntermediateDecimalError::{LossyCast, OutOfRange, ParseError};
89
use bigdecimal::{num_bigint::BigInt, BigDecimal, ParseBigDecimalError, ToPrimitive};
910
use serde::{Deserialize, Serialize};
1011
use std::{fmt, str::FromStr};
1112
use thiserror::Error;
1213

1314
/// Errors related to the processing of decimal values in proof-of-sql
1415
#[derive(Error, Debug, PartialEq)]
15-
pub enum DecimalError {
16+
pub enum IntermediateDecimalError {
1617
/// Represents an error encountered during the parsing of a decimal string.
1718
#[error(transparent)]
1819
ParseError(#[from] ParseBigDecimalError),
@@ -27,6 +28,8 @@ pub enum DecimalError {
2728
ConversionFailure,
2829
}
2930

31+
impl Eq for IntermediateDecimalError {}
32+
3033
/// An intermediate placeholder for a decimal
3134
#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
3235
pub struct IntermediateDecimal {
@@ -55,10 +58,10 @@ impl IntermediateDecimal {
5558
&self,
5659
precision: u8,
5760
scale: i8,
58-
) -> Result<BigInt, DecimalError> {
61+
) -> Result<BigInt, IntermediateDecimalError> {
5962
let scaled_decimal = self.value.with_scale(scale.into());
6063
if scaled_decimal.digits() > precision.into() {
61-
return Err(DecimalError::LossyCast);
64+
return Err(LossyCast);
6265
}
6366
let (d, _) = scaled_decimal.into_bigint_and_exponent();
6467
Ok(d)
@@ -72,14 +75,14 @@ impl fmt::Display for IntermediateDecimal {
7275
}
7376

7477
impl FromStr for IntermediateDecimal {
75-
type Err = DecimalError;
78+
type Err = IntermediateDecimalError;
7679

7780
fn from_str(decimal_string: &str) -> Result<Self, Self::Err> {
7881
BigDecimal::from_str(decimal_string)
7982
.map(|value| IntermediateDecimal {
8083
value: value.normalized(),
8184
})
82-
.map_err(DecimalError::ParseError)
85+
.map_err(ParseError)
8386
}
8487
}
8588

@@ -100,47 +103,47 @@ impl From<i64> for IntermediateDecimal {
100103
}
101104

102105
impl TryFrom<&str> for IntermediateDecimal {
103-
type Error = DecimalError;
106+
type Error = IntermediateDecimalError;
104107

105108
fn try_from(s: &str) -> Result<Self, Self::Error> {
106109
IntermediateDecimal::from_str(s)
107110
}
108111
}
109112

110113
impl TryFrom<String> for IntermediateDecimal {
111-
type Error = DecimalError;
114+
type Error = IntermediateDecimalError;
112115

113116
fn try_from(s: String) -> Result<Self, Self::Error> {
114117
IntermediateDecimal::from_str(&s)
115118
}
116119
}
117120

118121
impl TryFrom<IntermediateDecimal> for i128 {
119-
type Error = DecimalError;
122+
type Error = IntermediateDecimalError;
120123

121124
fn try_from(decimal: IntermediateDecimal) -> Result<Self, Self::Error> {
122125
if !decimal.value.is_integer() {
123-
return Err(DecimalError::LossyCast);
126+
return Err(LossyCast);
124127
}
125128

126129
match decimal.value.to_i128() {
127130
Some(value) if (i128::MIN..=i128::MAX).contains(&value) => Ok(value),
128-
_ => Err(DecimalError::OutOfRange),
131+
_ => Err(OutOfRange),
129132
}
130133
}
131134
}
132135

133136
impl TryFrom<IntermediateDecimal> for i64 {
134-
type Error = DecimalError;
137+
type Error = IntermediateDecimalError;
135138

136139
fn try_from(decimal: IntermediateDecimal) -> Result<Self, Self::Error> {
137140
if !decimal.value.is_integer() {
138-
return Err(DecimalError::LossyCast);
141+
return Err(LossyCast);
139142
}
140143

141144
match decimal.value.to_i64() {
142145
Some(value) if (i64::MIN..=i64::MAX).contains(&value) => Ok(value),
143-
_ => Err(DecimalError::OutOfRange),
146+
_ => Err(OutOfRange),
144147
}
145148
}
146149
}
@@ -195,10 +198,7 @@ mod tests {
195198
let overflow_decimal = IntermediateDecimal {
196199
value: BigDecimal::from_str("170141183460469231731687303715884105728").unwrap(),
197200
};
198-
assert_eq!(
199-
i128::try_from(overflow_decimal),
200-
Err(DecimalError::OutOfRange)
201-
);
201+
assert_eq!(i128::try_from(overflow_decimal), Err(OutOfRange));
202202

203203
let valid_decimal_negative = IntermediateDecimal {
204204
value: BigDecimal::from_str("-170141183460469231731687303715884105728").unwrap(),
@@ -211,7 +211,7 @@ mod tests {
211211
let non_integer = IntermediateDecimal {
212212
value: BigDecimal::from_str("100.5").unwrap(),
213213
};
214-
assert_eq!(i128::try_from(non_integer), Err(DecimalError::LossyCast));
214+
assert_eq!(i128::try_from(non_integer), Err(LossyCast));
215215
}
216216

217217
#[test]
@@ -229,10 +229,7 @@ mod tests {
229229
let overflow_decimal = IntermediateDecimal {
230230
value: BigDecimal::from_str("9223372036854775808").unwrap(),
231231
};
232-
assert_eq!(
233-
i64::try_from(overflow_decimal),
234-
Err(DecimalError::OutOfRange)
235-
);
232+
assert_eq!(i64::try_from(overflow_decimal), Err(OutOfRange));
236233

237234
let valid_decimal_negative = IntermediateDecimal {
238235
value: BigDecimal::from_str("-9223372036854775808").unwrap(),
@@ -245,6 +242,6 @@ mod tests {
245242
let non_integer = IntermediateDecimal {
246243
value: BigDecimal::from_str("100.5").unwrap(),
247244
};
248-
assert_eq!(i64::try_from(non_integer), Err(DecimalError::LossyCast));
245+
assert_eq!(i64::try_from(non_integer), Err(LossyCast));
249246
}
250247
}

‎crates/proof-of-sql/src/base/math/decimal.rs

+61-19
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,51 @@
11
//! Module for parsing an `IntermediateDecimal` into a `Decimal75`.
22
use crate::{
3-
base::scalar::Scalar,
4-
sql::parse::{ConversionError, ConversionResult},
3+
base::{
4+
math::decimal::DecimalError::{
5+
IntermediateDecimalConversionError, InvalidPrecision, RoundingError,
6+
},
7+
scalar::Scalar,
8+
},
9+
sql::parse::{
10+
ConversionError::{self, DecimalConversionError},
11+
ConversionResult,
12+
},
513
};
6-
use proof_of_sql_parser::intermediate_decimal::IntermediateDecimal;
14+
use proof_of_sql_parser::intermediate_decimal::{IntermediateDecimal, IntermediateDecimalError};
715
use serde::{Deserialize, Deserializer, Serialize};
16+
use thiserror::Error;
17+
18+
/// Errors related to decimal operations.
19+
#[derive(Error, Debug, Eq, PartialEq)]
20+
pub enum DecimalError {
21+
#[error("Invalid decimal format or value: {0}")]
22+
/// Error when a decimal format or value is incorrect,
23+
/// the string isn't even a decimal e.g. "notastring",
24+
/// "-21.233.122" etc aka InvalidDecimal
25+
InvalidDecimal(String),
26+
27+
#[error("Decimal precision is not valid: {0}")]
28+
/// Decimal precision exceeds the allowed limit,
29+
/// e.g. precision above 75/76/whatever set by Scalar
30+
/// or non-positive aka InvalidPrecision
31+
InvalidPrecision(String),
32+
33+
#[error("Unsupported operation: cannot round decimal: {0}")]
34+
/// This error occurs when attempting to scale a
35+
/// decimal in such a way that a loss of precision occurs.
36+
RoundingError(String),
37+
38+
/// Errors that may occur when parsing an intermediate decimal
39+
/// into a posql decimal
40+
#[error("Intermediate decimal conversion error: {0}")]
41+
IntermediateDecimalConversionError(IntermediateDecimalError),
42+
}
43+
44+
impl From<IntermediateDecimalError> for ConversionError {
45+
fn from(err: IntermediateDecimalError) -> ConversionError {
46+
DecimalConversionError(IntermediateDecimalConversionError(err))
47+
}
48+
}
849

950
#[derive(Eq, PartialEq, Debug, Clone, Hash, Serialize, Copy)]
1051
/// limit-enforced precision
@@ -15,10 +56,10 @@ impl Precision {
1556
/// Constructor for creating a Precision instance
1657
pub fn new(value: u8) -> Result<Self, ConversionError> {
1758
if value > MAX_SUPPORTED_PRECISION || value == 0 {
18-
Err(ConversionError::PrecisionParseError(format!(
59+
Err(DecimalConversionError(InvalidPrecision(format!(
1960
"Failed to parse precision. Value of {} exceeds max supported precision of {}",
2061
value, MAX_SUPPORTED_PRECISION
21-
)))
62+
))))
2263
} else {
2364
Ok(Precision(value))
2465
}
@@ -73,9 +114,9 @@ impl<S: Scalar> Decimal<S> {
73114
) -> ConversionResult<Decimal<S>> {
74115
let scale_factor = new_scale - self.scale;
75116
if scale_factor < 0 || new_precision.value() < self.precision.value() + scale_factor as u8 {
76-
return Err(ConversionError::DecimalRoundingError(
117+
return Err(DecimalConversionError(RoundingError(
77118
"Scale factor must be non-negative".to_string(),
78-
));
119+
)));
79120
}
80121
let scaled_value = scale_scalar(self.value, scale_factor)?;
81122
Ok(Decimal::new(scaled_value, new_precision, new_scale))
@@ -86,14 +127,14 @@ impl<S: Scalar> Decimal<S> {
86127
const MINIMAL_PRECISION: u8 = 19;
87128
let raw_precision = precision.value();
88129
if raw_precision < MINIMAL_PRECISION {
89-
return Err(ConversionError::DecimalRoundingError(
130+
return Err(DecimalConversionError(RoundingError(
90131
"Precision must be at least 19".to_string(),
91-
));
132+
)));
92133
}
93134
if scale < 0 || raw_precision < MINIMAL_PRECISION + scale as u8 {
94-
return Err(ConversionError::DecimalRoundingError(
135+
return Err(DecimalConversionError(RoundingError(
95136
"Can not scale down a decimal".to_string(),
96-
));
137+
)));
97138
}
98139
let scaled_value = scale_scalar(S::from(&value), scale)?;
99140
Ok(Decimal::new(scaled_value, precision, scale))
@@ -104,14 +145,14 @@ impl<S: Scalar> Decimal<S> {
104145
const MINIMAL_PRECISION: u8 = 39;
105146
let raw_precision = precision.value();
106147
if raw_precision < MINIMAL_PRECISION {
107-
return Err(ConversionError::DecimalRoundingError(
148+
return Err(DecimalConversionError(RoundingError(
108149
"Precision must be at least 19".to_string(),
109-
));
150+
)));
110151
}
111152
if scale < 0 || raw_precision < MINIMAL_PRECISION + scale as u8 {
112-
return Err(ConversionError::DecimalRoundingError(
153+
return Err(DecimalConversionError(RoundingError(
113154
"Can not scale down a decimal".to_string(),
114-
));
155+
)));
115156
}
116157
let scaled_value = scale_scalar(S::from(&value), scale)?;
117158
Ok(Decimal::new(scaled_value, precision, scale))
@@ -132,8 +173,9 @@ impl<S: Scalar> Decimal<S> {
132173
/// * `target_scale` - The scale (number of decimal places) to use in the scalar.
133174
///
134175
/// ## Errors
135-
/// Returns `ConversionError::PrecisionParseError` if the number of digits in
136-
/// the decimal exceeds the `target_precision` after adjusting for `target_scale`.
176+
/// Returns `InvalidPrecision` error if the number of digits in
177+
/// the decimal exceeds the `target_precision` before or after adjusting for
178+
/// `target_scale`, or if the target precision is zero.
137179
pub(crate) fn try_into_to_scalar<S: Scalar>(
138180
d: &IntermediateDecimal,
139181
target_precision: Precision,
@@ -147,9 +189,9 @@ pub(crate) fn try_into_to_scalar<S: Scalar>(
147189
/// Note that we do not check for overflow.
148190
pub(crate) fn scale_scalar<S: Scalar>(s: S, scale: i8) -> ConversionResult<S> {
149191
if scale < 0 {
150-
return Err(ConversionError::DecimalRoundingError(
192+
return Err(DecimalConversionError(RoundingError(
151193
"Scale factor must be non-negative".to_string(),
152-
));
194+
)));
153195
}
154196
let ten = S::from(10);
155197
let mut res = s;

‎crates/proof-of-sql/src/base/scalar/mont_scalar.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
use super::{scalar_conversion_to_int, Scalar, ScalarConversionError};
2-
use crate::{base::math::decimal::MAX_SUPPORTED_PRECISION, sql::parse::ConversionError};
2+
use crate::{
3+
base::{
4+
math::decimal::{DecimalError, MAX_SUPPORTED_PRECISION},
5+
scalar::mont_scalar::DecimalError::InvalidDecimal,
6+
},
7+
sql::parse::{ConversionError, ConversionError::DecimalConversionError},
8+
};
39
use ark_ff::{BigInteger, Field, Fp, Fp256, MontBackend, MontConfig, PrimeField};
410
use ark_serialize::{CanonicalDeserialize, CanonicalSerialize};
511
use bytemuck::TransparentWrapper;
@@ -163,11 +169,11 @@ impl<T: MontConfig<4>> TryFrom<num_bigint::BigInt> for MontScalar<T> {
163169

164170
// Check if the number of digits exceeds the maximum precision allowed
165171
if digits.len() > MAX_SUPPORTED_PRECISION.into() {
166-
return Err(ConversionError::InvalidDecimal(format!(
172+
return Err(DecimalConversionError(InvalidDecimal(format!(
167173
"Attempted to parse a number with {} digits, which exceeds the max supported precision of {}",
168174
digits.len(),
169175
MAX_SUPPORTED_PRECISION
170-
)));
176+
))));
171177
}
172178

173179
// Continue with the previous logic

‎crates/proof-of-sql/src/sql/ast/comparison_util.rs

+15-4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
11
use crate::{
2-
base::{database::Column, math::decimal::Precision, scalar::Scalar},
3-
sql::parse::{type_check_binary_operation, ConversionError, ConversionResult},
2+
base::{
3+
database::Column,
4+
math::decimal::{DecimalError, Precision},
5+
scalar::Scalar,
6+
},
7+
sql::{
8+
ast::comparison_util::DecimalError::InvalidPrecision,
9+
parse::{
10+
type_check_binary_operation, ConversionError, ConversionError::DecimalConversionError,
11+
ConversionResult,
12+
},
13+
},
414
};
515
use bumpalo::Bump;
616
use proof_of_sql_parser::intermediate_ast::BinaryOperator;
@@ -67,8 +77,9 @@ pub(crate) fn scale_and_subtract<'a, S: Scalar>(
6777
rhs_precision_value + (max_scale - rhs_scale) as u8,
6878
);
6979
// Check if the precision is valid
70-
let _max_precision = Precision::new(max_precision_value)
71-
.map_err(|_| ConversionError::InvalidPrecision(max_precision_value as i16))?;
80+
let _max_precision = Precision::new(max_precision_value).map_err(|_| {
81+
DecimalConversionError(InvalidPrecision(max_precision_value.to_string()))
82+
})?;
7283
}
7384
unchecked_subtract_impl(
7485
alloc,

‎crates/proof-of-sql/src/sql/ast/numerical_util.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
use crate::{
22
base::{
33
database::{Column, ColumnType},
4-
math::decimal::{scale_scalar, Precision},
4+
math::decimal::{scale_scalar, DecimalError, Precision},
55
scalar::Scalar,
66
},
7-
sql::parse::{ConversionError, ConversionResult},
7+
sql::{
8+
ast::numerical_util::DecimalError::InvalidPrecision,
9+
parse::{ConversionError, ConversionError::DecimalConversionError, ConversionResult},
10+
},
811
};
912
use bumpalo::Bump;
1013

@@ -41,9 +44,10 @@ pub(crate) fn try_add_subtract_column_types(
4144
.max(right_precision_value - right_scale as i16)
4245
+ 1_i16;
4346
let precision = u8::try_from(precision_value)
44-
.map_err(|_| ConversionError::InvalidPrecision(precision_value))
47+
.map_err(|_| DecimalConversionError(InvalidPrecision(precision_value.to_string())))
4548
.and_then(|p| {
46-
Precision::new(p).map_err(|_| ConversionError::InvalidPrecision(p as i16))
49+
Precision::new(p)
50+
.map_err(|_| DecimalConversionError(InvalidPrecision(p.to_string())))
4751
})?;
4852
Ok(ColumnType::Decimal75(precision, scale))
4953
}

0 commit comments

Comments
 (0)