Skip to content

Commit

Permalink
chore: resolve clippy::pedantic lints in proof-of-sql-parser (#201)
Browse files Browse the repository at this point in the history
# Rationale for this change
We have `cargo clippy` running in our CI in order to enforce code
quality. In order to increase our standards, we should enable the
`clippy::pedantic` lint group.

# What changes are included in this PR?

This PR fixes clippy pedantic warnings for `proof-of-sql-parser` lib.

## Commit message structure

The commit messages in the PR adhere to the following structure:

`chore:` `autofix | manually fix` `lint_name` 
`lint_name` refers to clippy lint names as given here
(https://rust-lang.github.io/rust-clippy/master/index.html#/)

Are these changes tested?
Yes, `cargo clippy --lib -p proof-of-sql-parser -- -W clippy::pedantic`
runs without any warnings.
  • Loading branch information
JayWhite2357 authored Oct 3, 2024
2 parents 88e09d0 + 9cbe4d0 commit 04c42f3
Show file tree
Hide file tree
Showing 12 changed files with 147 additions and 47 deletions.
5 changes: 3 additions & 2 deletions crates/proof-of-sql-parser/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use alloc::string::String;
use snafu::Snafu;

/// Errors encountered during the parsing process
#[allow(clippy::module_name_repetitions)]
#[derive(Debug, Snafu, Eq, PartialEq)]
pub enum ParseError {
#[snafu(display("Unable to parse query"))]
Expand All @@ -17,13 +18,13 @@ pub enum ParseError {
error: String,
},
#[snafu(display("Unable to parse resource_id"))]
/// Can not parse the resource_id
/// Can not parse the `resource_id`
ResourceIdParseError {
/// The underlying error
error: String,
},
}

/// General parsing error that may occur, for example if the provided schema/object_name strings
/// General parsing error that may occur, for example if the provided `schema`/`object_name` strings
/// aren't valid postgres-style identifiers (excluding dollar signs).
pub type ParseResult<T> = Result<T, ParseError>;
17 changes: 12 additions & 5 deletions crates/proof-of-sql-parser/src/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,35 @@ pub struct Identifier {
impl Identifier {
/// Constructor for [Identifier]
///
/// Note: this constructor should be private within the proof_of_sql_parser crate.
/// Note: this constructor should be private within the `proof_of_sql_parser` crate.
/// This is necessary to guarantee that no one outside the crate
/// can create Names, thus securing that ResourceIds and Identifiers
/// can create Names, thus securing that [`ResourceId`]s and [`Identifier`]s
/// are always valid postgresql identifiers.
pub(crate) fn new<S: AsRef<str>>(string: S) -> Self {
Self {
name: ArrayString::from(&string.as_ref().to_lowercase()).expect("Identifier too long"),
}
}

/// An alias for [Identifier::from_str], provided for convenience.
/// An alias for [`Identifier::from_str`], provided for convenience.
///
/// # Errors
/// Returns a `ParseResult::Err` if the input string does not meet the requirements for a valid identifier.
/// This may include errors such as invalid characters or incorrect formatting based on the specific rules
/// that `Identifier::from_str` enforces.
pub fn try_new<S: AsRef<str>>(string: S) -> ParseResult<Self> {
Self::from_str(string.as_ref())
}

/// The name of this [Identifier]
/// It already implements [Deref] to [str], so this method is not necessary for most use cases.
#[must_use]
pub fn name(&self) -> &str {
self.name.as_str()
}

/// An alias for [Identifier::name], provided for convenience.
/// An alias for [`Identifier::name`], provided for convenience.
#[must_use]
pub fn as_str(&self) -> &str {
self.name()
}
Expand All @@ -46,7 +53,7 @@ impl FromStr for Identifier {
let name = IdentifierParser::new()
.parse(string)
.map_err(|e| ParseError::IdentifierParseError{ error:
format!("failed to parse identifier, (you may have used a reserved keyword as an ID, i.e. 'timestamp') {:?}", e)})?;
format!("failed to parse identifier, (you may have used a reserved keyword as an ID, i.e. 'timestamp') {e:?}")})?;

Ok(Identifier::new(name))
}
Expand Down
28 changes: 18 additions & 10 deletions crates/proof-of-sql-parser/src/intermediate_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use core::{
};
use serde::{Deserialize, Serialize};

/// Representation of a SetExpression, a collection of rows, each having one or more columns.
/// Representation of a `SetExpression`, a collection of rows, each having one or more columns.
#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
pub enum SetExpression {
/// Query result as `SetExpression`
Expand Down Expand Up @@ -50,6 +50,7 @@ pub struct AliasedResultExpr {

impl AliasedResultExpr {
/// Create a new `AliasedResultExpr`
#[must_use]
pub fn new(expr: Expression, alias: Identifier) -> Self {
Self {
expr: Box::new(expr),
Expand All @@ -59,6 +60,7 @@ impl AliasedResultExpr {

/// Try to get the identifier of the expression if it is a column
/// Otherwise return None
#[must_use]
pub fn try_as_identifier(&self) -> Option<&Identifier> {
match self.expr.as_ref() {
Expression::Column(column) => Some(column),
Expand Down Expand Up @@ -185,46 +187,52 @@ pub enum Expression {
}

impl Expression {
/// Create a new SUM()
/// Create a new `SUM()`
#[must_use]
pub fn sum(self) -> Box<Self> {
Box::new(Expression::Aggregation {
op: AggregationOperator::Sum,
expr: Box::new(self),
})
}

/// Create a new MAX()
/// Create a new `MAX()`
#[must_use]
pub fn max(self) -> Box<Self> {
Box::new(Expression::Aggregation {
op: AggregationOperator::Max,
expr: Box::new(self),
})
}

/// Create a new MIN()
/// Create a new `MIN()`
#[must_use]
pub fn min(self) -> Box<Self> {
Box::new(Expression::Aggregation {
op: AggregationOperator::Min,
expr: Box::new(self),
})
}

/// Create a new COUNT()
/// Create a new `COUNT()`
#[must_use]
pub fn count(self) -> Box<Self> {
Box::new(Expression::Aggregation {
op: AggregationOperator::Count,
expr: Box::new(self),
})
}

/// Create a new FIRST()
/// Create a new `FIRST()`
#[must_use]
pub fn first(self) -> Box<Self> {
Box::new(Expression::Aggregation {
op: AggregationOperator::First,
expr: Box::new(self),
})
}
/// Create an `AliasedResultExpr` from an `Expression` using the provided alias.
#[must_use]
pub fn alias(self, alias: &str) -> AliasedResultExpr {
AliasedResultExpr {
expr: Box::new(self),
Expand Down Expand Up @@ -277,7 +285,7 @@ impl core::ops::Sub<Box<Expression>> for Box<Expression> {
}
}

/// OrderBy
/// `OrderBy`
#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
pub struct OrderBy {
/// which column to order by
Expand All @@ -286,7 +294,7 @@ pub struct OrderBy {
pub direction: OrderByDirection,
}

/// OrderByDirection values
/// `OrderByDirection` values
#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
pub enum OrderByDirection {
/// Ascending
Expand All @@ -310,7 +318,7 @@ impl Display for OrderByDirection {
pub struct Slice {
/// number of rows to return
///
/// if u64::MAX, specify all rows
/// if `u64::MAX`, specify all rows
pub number_rows: u64,

/// number of rows to skip
Expand Down Expand Up @@ -349,7 +357,7 @@ macro_rules! impl_int_to_literal {
($tt:ty) => {
impl From<$tt> for Literal {
fn from(val: $tt) -> Self {
Literal::BigInt(val as i64)
Literal::BigInt(i64::from(val))
}
}
};
Expand Down
17 changes: 15 additions & 2 deletions crates/proof-of-sql-parser/src/intermediate_decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use serde::{Deserialize, Serialize};
use snafu::Snafu;

/// Errors related to the processing of decimal values in proof-of-sql
#[allow(clippy::module_name_repetitions)]
#[derive(Snafu, Debug, PartialEq)]
pub enum IntermediateDecimalError {
/// Represents an error encountered during the parsing of a decimal string.
Expand Down Expand Up @@ -47,22 +48,34 @@ pub struct IntermediateDecimal {

impl IntermediateDecimal {
/// Get the integer part of the fixed-point representation of this intermediate decimal.
#[must_use]
pub fn value(&self) -> BigDecimal {
self.value.clone()
}

/// Get the precision of the fixed-point representation of this intermediate decimal.
#[must_use]
pub fn precision(&self) -> u8 {
self.value.digits() as u8
match u8::try_from(self.value.digits()) {
Ok(v) => v,
Err(_) => u8::MAX, // Returning u8::MAX on truncation
}
}

/// Get the scale of the fixed-point representation of this intermediate decimal.
#[must_use]
pub fn scale(&self) -> i8 {
self.value.fractional_digit_count() as i8
match i8::try_from(self.value.fractional_digit_count()) {
Ok(v) => v,
Err(_) => i8::MAX, // Returning i8::MAX on truncation
}
}

/// Attempts to convert the decimal to `BigInt` while adjusting it to the specified precision and scale.
/// Returns an error if the conversion cannot be performed due to precision or scale constraints.
///
/// # Errors
/// Returns an `IntermediateDecimalError::LossyCast` error if the number of digits in the scaled decimal exceeds the specified precision.
pub fn try_into_bigint_with_precision_and_scale(
&self,
precision: u8,
Expand Down
5 changes: 3 additions & 2 deletions crates/proof-of-sql-parser/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![doc = include_str!("../README.md")]
#![no_std]
#![allow(clippy::missing_panics_doc)] // Fixed in Issue #163
extern crate alloc;

/// Module for handling an intermediate decimal type received from the lexer.
Expand Down Expand Up @@ -34,9 +35,9 @@ pub mod resource_id;
pub use resource_id::ResourceId;

// lalrpop-generated code is not clippy-compliant
lalrpop_mod!(#[allow(clippy::all, missing_docs, clippy::missing_docs_in_private_items)] pub sql);
lalrpop_mod!(#[allow(clippy::all, missing_docs, clippy::missing_docs_in_private_items, clippy::pedantic)] pub sql);

/// Implement Deserialize through FromStr to avoid invalid identifiers.
/// Implement [`Deserialize`] through [`FromStr`] to avoid invalid identifiers.
#[macro_export]
macro_rules! impl_serde_from_str {
($type:ty) => {
Expand Down
3 changes: 2 additions & 1 deletion crates/proof-of-sql-parser/src/posql_time/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize};
use snafu::Snafu;

/// Errors related to time operations, including timezone and timestamp conversions.s
#[allow(clippy::module_name_repetitions)]
#[derive(Snafu, Debug, Eq, PartialEq, Serialize, Deserialize)]
pub enum PoSQLTimestampError {
/// Error when the timezone string provided cannot be parsed into a valid timezone.
Expand Down Expand Up @@ -44,7 +45,7 @@ pub enum PoSQLTimestampError {
error: String,
},

/// Represents a failure to parse a provided time unit precision value, PoSQL supports
/// Represents a failure to parse a provided time unit precision value, `PoSQL` supports
/// Seconds, Milliseconds, Microseconds, and Nanoseconds
#[snafu(display("Timestamp parsing error: {error}"))]
UnsupportedPrecision {
Expand Down
30 changes: 26 additions & 4 deletions crates/proof-of-sql-parser/src/posql_time/timestamp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use core::hash::Hash;
use serde::{Deserialize, Serialize};

/// Represents a fully parsed timestamp with detailed time unit and timezone information
#[allow(clippy::module_name_repetitions)]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)]
pub struct PoSQLTimestamp {
/// The datetime representation in UTC.
Expand All @@ -19,21 +20,24 @@ pub struct PoSQLTimestamp {

impl PoSQLTimestamp {
/// Returns the combined date and time with time zone.
#[must_use]
pub fn timestamp(&self) -> DateTime<Utc> {
self.timestamp
}

/// Returns the [PoSQLTimeUnit] for this timestamp
/// Returns the [`PoSQLTimeUnit`] for this timestamp
#[must_use]
pub fn timeunit(&self) -> PoSQLTimeUnit {
self.timeunit
}

/// Returns the [PoSQLTimeZone] for this timestamp
/// Returns the [`PoSQLTimeZone`] for this timestamp
#[must_use]
pub fn timezone(&self) -> PoSQLTimeZone {
self.timezone
}

/// Attempts to parse a timestamp string into an [PoSQLTimestamp] structure.
/// Attempts to parse a timestamp string into an [`PoSQLTimestamp`] structure.
/// This function supports two primary formats:
///
/// 1. **RFC 3339 Parsing**:
Expand All @@ -45,6 +49,13 @@ impl PoSQLTimestamp {
/// - The `from_offset` method is used to determine whether the timezone should be represented
/// as `Utc` or `FixedOffset`. This function simplifies the decision based on the offset value.
///
/// # Errors
/// This function returns a `PoSQLTimestampError` in the following cases:
///
/// - **Parsing Error**: Returns `PoSQLTimestampError::ParsingError` if the input string does not conform
/// to the RFC 3339 format or if the timestamp cannot be parsed due to invalid formatting.
/// This error includes the original parsing error message for further details.
///
/// # Examples
/// ```
/// use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -94,6 +105,17 @@ impl PoSQLTimestamp {
/// - Since Unix epoch timestamps don't inherently carry timezone information,
/// any Unix time parsed directly from an integer is assumed to be in UTC.
///
/// # Errors
/// This function returns a `PoSQLTimestampError` in the following cases:
///
/// - **Ambiguous Time**: Returns `PoSQLTimestampError::Ambiguous` if the provided epoch time
/// corresponds to a time that is ambiguous (e.g., during a daylight saving time change where
/// the local time could correspond to two different UTC times).
///
/// - **Non-Existent Local Time**: Returns `PoSQLTimestampError::LocalTimeDoesNotExist` if the
/// provided epoch time corresponds to a time that does not exist in the local time zone (e.g.,
/// during a daylight saving time change where a certain local time is skipped).
///
/// # Examples
/// ```
/// use chrono::{DateTime, Utc};
Expand All @@ -112,7 +134,7 @@ impl PoSQLTimestamp {
timezone: PoSQLTimeZone::Utc,
}),
LocalResult::Ambiguous(earliest, latest) => Err(PoSQLTimestampError::Ambiguous{ error:
format!("The local time is ambiguous because there is a fold in the local time: earliest: {} latest: {} ", earliest, latest),
format!("The local time is ambiguous because there is a fold in the local time: earliest: {earliest} latest: {latest} "),
}),
LocalResult::None => Err(PoSQLTimestampError::LocalTimeDoesNotExist),
}
Expand Down
5 changes: 3 additions & 2 deletions crates/proof-of-sql-parser/src/posql_time/timezone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ use serde::{Deserialize, Serialize};
pub enum PoSQLTimeZone {
/// Default variant for UTC timezone
Utc,
/// TImezone offset in seconds
/// `TImezone` offset in seconds
FixedOffset(i32),
}

impl PoSQLTimeZone {
/// Parse a timezone from a count of seconds
#[must_use]
pub fn from_offset(offset: i32) -> Self {
if offset == 0 {
PoSQLTimeZone::Utc
Expand Down Expand Up @@ -67,7 +68,7 @@ impl fmt::Display for PoSQLTimeZone {
if seconds < 0 {
write!(f, "-{:02}:{:02}", hours.abs(), minutes)
} else {
write!(f, "+{:02}:{:02}", hours, minutes)
write!(f, "+{hours:02}:{minutes:02}")
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/proof-of-sql-parser/src/posql_time/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use core::fmt;
use serde::{Deserialize, Serialize};

/// An intermediate type representing the time units from a parsed query
#[allow(clippy::module_name_repetitions)]
#[derive(Debug, Clone, Copy, Hash, Serialize, Deserialize, PartialEq, Eq)]
pub enum PoSQLTimeUnit {
/// Represents seconds with precision 0: ex "2024-06-20 12:34:56"
Expand Down
Loading

0 comments on commit 04c42f3

Please sign in to comment.