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

Seal the decimal type. #2439

Closed
wants to merge 2 commits into from
Closed

Conversation

HaoYang670
Copy link
Contributor

@HaoYang670 HaoYang670 commented Aug 14, 2022

Which issue does this PR close?

Closes #2440.

Rationale for this change

The current implementation of BasicDecimal cannot restrict the BYTE_WIDTH to be 16 or 32, which means that you can create an invalid decimal value like this

let v = BasicDecimal::<1>::new(1, 1, &[1]);

What changes are included in this PR?

We create a new public trait ValidDecimal and implement the trait for Decimal128 and Decimal256. The idea is from https://users.rust-lang.org/t/how-to-seal-the-const-generic/77947/2.
Because of the orphan rule, downstream users cannot implement ValidDecimal for other BasicDecimal types.

Are there any user-facing changes?

No breaking.

Signed-off-by: remzi <13716567376yh@gmail.com>
Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 14, 2022
@HaoYang670
Copy link
Contributor Author

Hi @tustvold, Could you please help review?

@HaoYang670
Copy link
Contributor Author

@viirya @liukun4515 Could you please help to review?

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

As #2001 hasn't been released yet, I think we have an opportunity to revisit this prior to the next release #2382 (comment)

In particular using a const generic and then constraining it in this way feels a bit derived. Perhaps we could instead have

pub trait DecimalType {
    const BYTE_LENGTH: usize;
    const MAX_PRECISION: usize;
    const MAX_SCALE: usize;
    const TYPE_CONSTRUCTOR: fn(usize, usize) -> DataType;
    const DEFAULT_TYPE: DataType;
}

pub struct Decimal128Type {}

pub struct Decimal256Type {}

pub struct DecimalArray<T: DecimalType>

@HaoYang670
Copy link
Contributor Author

Hi @tustvold, do you mean that you want to revert #2383?

@tustvold
Copy link
Contributor

Hi @tustvold, do you mean that you want to revert #2383?

I was more wondering if we could build upon it, to polish off some rough edges. Perhaps you could take a look at #2477 and let me know what you think?

@HaoYang670 HaoYang670 closed this Aug 18, 2022
@HaoYang670 HaoYang670 deleted the seal_decimal branch August 18, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is no compiler error when using an invalid Decimal type.
2 participants