You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/09 10:22:06 UTC

[GitHub] [arrow-rs] HaoYang670 opened a new issue, #2852: More consideration to `Decimal(precision = 0, ..)`

HaoYang670 opened a new issue, #2852:
URL: https://github.com/apache/arrow-rs/issues/2852

   **Which part is this question about**
   <!--
   Is it code base, library api, documentation or some other part?
   -->
   The definition of the `Decimal` type.
   
   **Describe your question**
   <!--
   A clear and concise description of what the question is.
   -->
   The current definition of the decimal type is `Decimal(u8, u8)`, which allows the precision to be zero.
   In the Datafusion, we meet as error which the zero precision leads the system to panic: https://github.com/apache/arrow-datafusion/issues/3665.
   
   My questions are:
   1. Does `precision = 0` make sense? 
   2. If it is, what is the value of `Decimal(0, ..)`?
   3. If it is not, how should we avoid the zero precision? Here are 2 ways I think:
       a. Add runtime check to all decimal functions when creating a new decimal type.
       b. Using the `NonZeroU8` to represent precision: https://doc.rust-lang.org/std/num/struct.NonZeroU8.html. Actually, `NonZeroU8` also does runtime checking when creating a value. But the benefit is that the type system help us find where to add the checking. The disadvantage it that we will change the API.
   
   **Additional context**
   <!--
   Add any other context about the problem here.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] liukun4515 commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1272780719

   In the datafusion, we can check the value of precision in the SQL level, and make sure the input precision and scale is valid.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] HaoYang670 commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1272811087

   > In the datafusion, we can check the value of precision in the SQL level, and make sure the input precision and scale is valid.
   
   If we can check all `precision = 0` cases during the constant folding stage in Datafusion, then it is great. 👍
   
   However, as this is a bug in arrow-rs, we should also add the zero checking here, because arrow-rs is open to all projects, not only the Datafusion.
   
   My suggestion is to use `NonZeroU8`, which can avoid duplicated checkings. (avoid checking twice in Datafusion and arrow-rs)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] psvri commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
psvri commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1323907770

   Hello,
   
   Should we rather refactor the function [`validate_precision_scale`](https://github.com/apache/arrow-rs/blob/master/arrow-array/src/array/primitive_array.rs#L1023) into [`DecimalType`](https://github.com/apache/arrow-rs/blob/master/arrow-array/src/types.rs#L489) . This way we can avoid code duplication in datafusion and expose it to others as well.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] psvri commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
psvri commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1323930941

   Okay, I will do the refactor in another PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] HaoYang670 commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1272509120

   cc @tustvold @liukun4515 @viirya. PTAL. Thank you!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] More consideration to `Decimal(precision = 0, ..)` [arrow-rs]

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed issue #2852: More consideration to `Decimal(precision = 0, ..)`
URL: https://github.com/apache/arrow-rs/issues/2852


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] tustvold commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1272524083

   We already have validation that checks if the precision is too high, I think we should just extend that logic. I don't think encoding it in the type system is necessary


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] More consideration to `Decimal(precision = 0, ..)` [arrow-rs]

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-2002168188

   Closed by #3162


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] tustvold commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1323911508

   That sounds like a good idea to me :+1: 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] liukun4515 commented on issue #2852: More consideration to `Decimal(precision = 0, ..)`

Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #2852:
URL: https://github.com/apache/arrow-rs/issues/2852#issuecomment-1272781037

   I don't see any system use the `precision = 0`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org