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 2021/06/14 08:02:26 UTC
[GitHub] [arrow-rs] alippai opened a new pull request #453: Add C data interface for decimal128
alippai opened a new pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #413.
# Rationale for this change
It's missing from the current API
# What changes are included in this PR?
Handling of Decimal on the FFI is implemented
# Are there any user-facing changes?
New datatype is added to a current interface
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao commented on a change in pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r656113144
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
Review comment:
If we allow this, we open the door to allow variations of the spec in order to be robust. imo this makes the spec weaker: producers can "get away" with variations of the spec when using Rust, but will fail in e.g. C++. imo this defeats the purpose of a spec and the Arrow project, as interoperability is not no longer guaranteed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao commented on a change in pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r654790603
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
+ .collect::<Vec<&str>>()
+ .as_slice()
+ {
+ ["d", precision, scale] => {
+ let parsed_precision = precision.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
+ other
+ ))
+ })?;
+ let parsed_scale = scale.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
+ other
+ ))
+ })?;
+ DataType::Decimal(parsed_precision, parsed_scale)
+ }
+ ["d", precision, scale, bits] => {
+ if *bits != "128" {
+ return Err(ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is still not supported in Rust implementation",
+ other
+ )));
+ }
+ let parsed_precision = precision.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
Review comment:
This happens when the type is incorrect, right? Shouldn't we return a different message here? Something like "The decimal type requires an integer precision"? Same for the scale.
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
Review comment:
It is a cool ideal, but I believe that this also accepts something like `d:1:2`, which is not in spec.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on a change in pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
alippai commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r654795387
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
Review comment:
Yes, I was following https://en.wikipedia.org/wiki/Robustness_principle#:~:text=In%20computing%2C%20the%20robustness%20principle,liberal%20in%20what%20you%20accept%22. , but I can change it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao merged pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
jorgecarleitao merged pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (303b666) into [master](https://codecov.io/gh/apache/arrow-rs/commit/60fedf7011e15578b2f5e6fd6e3486f4e01b2ee2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60fedf7) will **decrease** coverage by `0.00%`.
> The diff coverage is `65.62%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.66% 82.65% -0.01%
==========================================
Files 165 165
Lines 45550 45580 +30
==========================================
+ Hits 37653 37676 +23
- Misses 7897 7904 +7
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `82.00% <65.62%> (-0.86%)` | :arrow_down: |
| [arrow/src/array/transform/boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9ib29sZWFuLnJz) | `76.92% <0.00%> (-7.70%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `95.04% <0.00%> (+0.19%)` | :arrow_up: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [60fedf7...303b666](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860251015
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9fa63d2) into [master](https://codecov.io/gh/apache/arrow-rs/commit/60fedf7011e15578b2f5e6fd6e3486f4e01b2ee2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60fedf7) will **decrease** coverage by `0.04%`.
> The diff coverage is `58.75%`.
> :exclamation: Current head 9fa63d2 differs from pull request most recent head 04c3d67. Consider uploading reports for the commit 04c3d67 to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.66% 82.61% -0.05%
==========================================
Files 165 165
Lines 45550 45598 +48
==========================================
+ Hits 37653 37672 +19
- Misses 7897 7926 +29
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `79.14% <56.57%> (-3.71%)` | :arrow_down: |
| [arrow/src/array/array\_struct.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3N0cnVjdC5ycw==) | `87.84% <100.00%> (-0.59%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `85.78% <100.00%> (-0.13%)` | :arrow_down: |
| [arrow/src/array/transform/boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9ib29sZWFuLnJz) | `76.92% <0.00%> (-7.70%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `95.04% <0.00%> (+0.19%)` | :arrow_up: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [60fedf7...04c3d67](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (030bb62) into [master](https://codecov.io/gh/apache/arrow-rs/commit/1f1c63729955d87a0083719f87b651a9c9adc6b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f1c637) will **decrease** coverage by `0.00%`.
> The diff coverage is `65.62%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.65% 82.65% -0.01%
==========================================
Files 165 165
Lines 45563 45593 +30
==========================================
+ Hits 37662 37684 +22
- Misses 7901 7909 +8
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `82.00% <65.62%> (-0.86%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `94.85% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1f1c637...030bb62](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a1de09) into [master](https://codecov.io/gh/apache/arrow-rs/commit/e21f576e5a01577f63f7d19a0e921417eb14b8db?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e21f576) will **decrease** coverage by `0.00%`.
> The diff coverage is `65.62%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.64% 82.63% -0.01%
==========================================
Files 164 164
Lines 45508 45538 +30
==========================================
+ Hits 37609 37632 +23
- Misses 7899 7906 +7
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `82.00% <65.62%> (-0.86%)` | :arrow_down: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e21f576...4a1de09](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-864589683
If you don't mind, I added an extra date32 integration test and timestamp support 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on a change in pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
alippai commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r654796610
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
+ .collect::<Vec<&str>>()
+ .as_slice()
+ {
+ ["d", precision, scale] => {
+ let parsed_precision = precision.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
+ other
+ ))
+ })?;
+ let parsed_scale = scale.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
+ other
+ ))
+ })?;
+ DataType::Decimal(parsed_precision, parsed_scale)
+ }
+ ["d", precision, scale, bits] => {
+ if *bits != "128" {
+ return Err(ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is still not supported in Rust implementation",
+ other
+ )));
+ }
+ let parsed_precision = precision.parse::<usize>().map_err(|_| {
+ ArrowError::CDataInterface(format!(
+ "The decimal \"{:?}\" is not supported in Rust implementation",
Review comment:
Updated
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-866591737
@jorgecarleitao @alamb all comments are addressed, I think this is ready now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on a change in pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
alippai commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r656129078
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
Review comment:
Makes sense. Updated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on a change in pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
alippai commented on a change in pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#discussion_r650589924
##########
File path: arrow/src/ffi.rs
##########
@@ -271,12 +271,54 @@ fn to_field(schema: &FFI_ArrowSchema) -> Result<Field> {
.collect::<Result<Vec<_>>>()?;
DataType::Struct(children)
}
- other => {
- return Err(ArrowError::CDataInterface(format!(
- "The datatype \"{:?}\" is still not supported in Rust implementation",
- other
- )))
- }
+ other => match other
+ .split(|c| c == ':' || c == ',')
Review comment:
Not sure if this is robust or ugly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (47c3c8c) into [master](https://codecov.io/gh/apache/arrow-rs/commit/60fedf7011e15578b2f5e6fd6e3486f4e01b2ee2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60fedf7) will **decrease** coverage by `0.01%`.
> The diff coverage is `58.33%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.66% 82.65% -0.02%
==========================================
Files 165 165
Lines 45550 45584 +34
==========================================
+ Hits 37653 37676 +23
- Misses 7897 7908 +11
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `81.27% <58.33%> (-1.58%)` | :arrow_down: |
| [arrow/src/array/transform/boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9ib29sZWFuLnJz) | `76.92% <0.00%> (-7.70%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `95.04% <0.00%> (+0.19%)` | :arrow_up: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [60fedf7...47c3c8c](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #453: Add C data interface for decimal128 and timestamp
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/arrow-rs/pull/453#issuecomment-860252582
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#453](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4201ae4) into [master](https://codecov.io/gh/apache/arrow-rs/commit/4c7d4189e72901a78fb4f4250c11421241dd9e13?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c7d418) will **decrease** coverage by `0.04%`.
> The diff coverage is `54.43%`.
> :exclamation: Current head 4201ae4 differs from pull request most recent head 610ff6c. Consider uploading reports for the commit 610ff6c to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 82.65% 82.61% -0.05%
==========================================
Files 165 165
Lines 45524 45602 +78
==========================================
+ Hits 37628 37673 +45
- Misses 7896 7929 +33
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `78.71% <54.43%> (-4.15%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `94.85% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/453/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.93% <0.00%> (+0.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4c7d418...610ff6c](https://codecov.io/gh/apache/arrow-rs/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org