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/11/29 10:52:57 UTC
[GitHub] [arrow] dragosmg opened a new pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
dragosmg opened a new pull request #11790:
URL: https://github.com/apache/arrow/pull/11790
This PR looks to swap `decimal()` for `decimal128()` in preparation for adding `decimal256()`. At present both `decimal()` and `decimal128()` work, but we might decide to deprecate `decimal()` at some point in the future.
--
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] dragosmg commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981624629
There's also [this reference](https://github.com/apache/arrow/blob/236362aaf5bdee0eb88da5048eeda174182ef508/r/src/array_to_vector.cpp#L1219), which might need updating.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759094997
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Row 400 should be checked as that comes from the R code. I'd be in favour of keeping all of them.
Rows 399 & 401: same as for the other arg to `decimal128()`, this is a generic R error: `Error in decimal128(4) : argument "scale" is missing, with no default`. I merely duplicated the tests we had for `decimal()`. Given it's the rather generic R error, I'd say assertion on content might not be needed.
--
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] dragosmg commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981528911
Do we need to change this to point to `DECIMAL128`?
https://github.com/apache/arrow/blob/8581f36a81df3b03f9e58ac1cb5fdfc473e61352/r/src/datatype.cpp#L83
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Buildkite builds:
[Finished] <https://buildkite.com/ursa-computing/benchmarks-ec2-t3-xlarge-us-east-2/builds/1646| `310fca93` ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/ursa-computing/benchmarks-ursa-i9-9960x/builds/1649| `310fca93` ursa-i9-9960x>
[Finished] <https://buildkite.com/ursa-computing/benchmarks-ursa-thinkcentre-m75q/builds/1587| `310fca93` ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/ursa-computing/benchmarks-ec2-t3-xlarge-us-east-2/builds/1645| `ee32aeaa` ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/ursa-computing/benchmarks-ursa-i9-9960x/builds/1648| `ee32aeaa` ursa-i9-9960x>
[Finished] <https://buildkite.com/ursa-computing/benchmarks-ursa-thinkcentre-m75q/builds/1586| `ee32aeaa` ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r761092011
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -385,16 +385,20 @@ test_that("DictionaryType validation", {
})
test_that("decimal type and validation", {
- expect_error(decimal())
+ expect_r6_class(decimal(4, 2), "Decimal128Type")
+
expect_error(decimal("four"), '"precision" must be an integer')
- expect_error(decimal(4))
expect_error(decimal(4, "two"), '"scale" must be an integer')
expect_error(decimal(NA, 2), '"precision" must be an integer')
- expect_error(decimal(0, 2), "Invalid: Decimal precision out of range [1, 38]: 0", fixed = TRUE)
- expect_error(decimal(100, 2), "Invalid: Decimal precision out of range [1, 38]: 100", fixed = TRUE)
expect_error(decimal(4, NA), '"scale" must be an integer')
- expect_r6_class(decimal(4, 2), "Decimal128Type")
+ expect_r6_class(decimal128(4, 2), "Decimal128Type")
+ expect_identical(class(decimal(2, 4)), class(decimal128(2, 4)))
Review comment:
Done!
--
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] dragosmg edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981624629
There's also this reference, which might need updating. https://github.com/apache/arrow/blob/236362aaf5bdee0eb88da5048eeda174182ef508/r/src/array_to_vector.cpp#L1219
--
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] thisisnic commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981526610
Looks good so far - out of interest, I see another ticket open for `decimal256()` - would it be possible to implement that 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
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dragosmg edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981527929
I think `decimal256()` is a bit more involved (not yet surfaced from C++). Hence the intention to implement them in 2 separate PRs. For `decimal128()` we didn't have to add a binding. It was merely a matter of porting the code for `decimal()`.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759128976
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
Review comment:
Sure. Done
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r761088741
##########
File path: r/tests/testthat/test-dplyr-funcs-type.R
##########
@@ -208,32 +208,42 @@ test_that("type checks with is() giving Arrow types", {
Table$create(
i32 = Array$create(1, int32()),
dec = Array$create(pi)$cast(decimal(3, 2)),
+ dec128 = Array$create(pi)$cast(decimal128(3, 2)),
f64 = Array$create(1.1, float64()),
str = Array$create("a", arrow::string())
) %>% transmute(
i32_is_i32 = is(i32, int32()),
i32_is_dec = is(i32, decimal(3, 2)),
+ i32_is_dec128 = is(i32, decimal128(3, 2)),
i32_is_i64 = is(i32, float64()),
i32_is_str = is(i32, arrow::string()),
dec_is_i32 = is(dec, int32()),
dec_is_dec = is(dec, decimal(3, 2)),
+ dec_is_dec128 = is(dec, decimal128(3, 2)),
dec_is_i64 = is(dec, float64()),
dec_is_str = is(dec, arrow::string()),
+ dec128_is_i32 = is(dec128, int32()),
+ dec128_is_dec128 = is(dec128, decimal128(3, 2)),
+ dec128_is_i64 = is(dec128, float64()),
+ dec128_is_str = is(dec128, arrow::string()),
f64_is_i32 = is(f64, int32()),
f64_is_dec = is(f64, decimal(3, 2)),
+ f64_is_dec128 = is(f64, decimal128(3, 2)),
f64_is_i64 = is(f64, float64()),
f64_is_str = is(f64, arrow::string()),
str_is_i32 = is(str, int32()),
str_is_dec = is(str, decimal(3, 2)),
+ str_is_dec128 = is(str, decimal128(3, 2)),
str_is_i64 = is(str, float64()),
str_is_str = is(str, arrow::string())
Review comment:
Done. I removed the testing for `decimal()` 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
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759096614
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range: 100")
+ expect_error(decimal128(4, NA), '"scale" must be an integer')
+
+ expect_r6_class(decimal128(4, 2), "Decimal128Type")
Review comment:
Done
--
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] ursabot commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759093687
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
Review comment:
It's the generic R error: `Error in decimal128() : argument "precision" is missing, with no default`. I'd say not crucial to assert the actual content.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759094997
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Row 400 should be checked as that comes from the body of the R function where we check the args. I'd be in favour of keeping all of them.
Rows 399 & 401: same as for the other arg to `decimal128()`, this is a generic R error: `Error in decimal128(4) : argument "scale" is missing, with no default`. I merely duplicated the tests we had for `decimal()`. Given it's the rather generic R error, I'd say assertion on content might not be needed.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759548927
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Done.
--
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] jonkeane commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759596467
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -385,16 +385,20 @@ test_that("DictionaryType validation", {
})
test_that("decimal type and validation", {
- expect_error(decimal())
+ expect_r6_class(decimal(4, 2), "Decimal128Type")
+
expect_error(decimal("four"), '"precision" must be an integer')
- expect_error(decimal(4))
expect_error(decimal(4, "two"), '"scale" must be an integer')
expect_error(decimal(NA, 2), '"precision" must be an integer')
- expect_error(decimal(0, 2), "Invalid: Decimal precision out of range [1, 38]: 0", fixed = TRUE)
- expect_error(decimal(100, 2), "Invalid: Decimal precision out of range [1, 38]: 100", fixed = TRUE)
expect_error(decimal(4, NA), '"scale" must be an integer')
- expect_r6_class(decimal(4, 2), "Decimal128Type")
+ expect_r6_class(decimal128(4, 2), "Decimal128Type")
+ expect_identical(class(decimal(2, 4)), class(decimal128(2, 4)))
Review comment:
We might want to include a comment above this to make super explicit that `decimal()` is an alias for `decimal128()`.
##########
File path: r/tests/testthat/test-dplyr-funcs-type.R
##########
@@ -208,32 +208,42 @@ test_that("type checks with is() giving Arrow types", {
Table$create(
i32 = Array$create(1, int32()),
dec = Array$create(pi)$cast(decimal(3, 2)),
+ dec128 = Array$create(pi)$cast(decimal128(3, 2)),
f64 = Array$create(1.1, float64()),
str = Array$create("a", arrow::string())
) %>% transmute(
i32_is_i32 = is(i32, int32()),
i32_is_dec = is(i32, decimal(3, 2)),
+ i32_is_dec128 = is(i32, decimal128(3, 2)),
i32_is_i64 = is(i32, float64()),
i32_is_str = is(i32, arrow::string()),
dec_is_i32 = is(dec, int32()),
dec_is_dec = is(dec, decimal(3, 2)),
+ dec_is_dec128 = is(dec, decimal128(3, 2)),
dec_is_i64 = is(dec, float64()),
dec_is_str = is(dec, arrow::string()),
+ dec128_is_i32 = is(dec128, int32()),
+ dec128_is_dec128 = is(dec128, decimal128(3, 2)),
+ dec128_is_i64 = is(dec128, float64()),
+ dec128_is_str = is(dec128, arrow::string()),
f64_is_i32 = is(f64, int32()),
f64_is_dec = is(f64, decimal(3, 2)),
+ f64_is_dec128 = is(f64, decimal128(3, 2)),
f64_is_i64 = is(f64, float64()),
f64_is_str = is(f64, arrow::string()),
str_is_i32 = is(str, int32()),
str_is_dec = is(str, decimal(3, 2)),
+ str_is_dec128 = is(str, decimal128(3, 2)),
str_is_i64 = is(str, float64()),
str_is_str = is(str, arrow::string())
Review comment:
I'm fine keeping both dec* and dec128* here, but technically they are all testing the same thing, right? If we were to clean this up, we should use dec128 for each of these (since we test elsewhere that `decimal(x, y) == decimal128(x, y)`
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] jonkeane closed pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #11790:
URL: https://github.com/apache/arrow/pull/11790
--
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] thisisnic commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981569260
> I think `decimal256()` is a bit more involved (not yet surfaced from C++). Hence the intention to implement them in 2 separate PRs. For `decimal128()` we didn't have to add a binding. It was merely a matter of porting the code for `decimal()`.
That's fair enough!
> Do we need to change this to point to DECIMAL128?
I think so - I took a look at how `TimeType` + `Time32` + `Time64` are all implemented here and in `arrow/r/R/type.R` to try to work this out.
I also found potential changes needed to the function `nse_funcs$is.numeric()`, and there's also this which could need updating: https://github.com/apache/arrow/blob/236362aaf5bdee0eb88da5048eeda174182ef508/r/R/enums.R#L68
--
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] dragosmg edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981528911
Do we need to change this to point to `DECIMAL128`?
https://github.com/apache/arrow/blob/8581f36a81df3b03f9e58ac1cb5fdfc473e61352/r/src/datatype.cpp#L83
I think there might be several changes needed in `datatype.cpp`.
--
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] dragosmg commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981527929
I think `decimal256()` is a bit more involved (not yet surfaced from C++). Hence the intention to implement them in 2 separate PRs.
--
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] paleolimbot commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981574397
I scanned this for typos and anything that looked off to me and found nothing!
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759149624
##########
File path: r/tests/testthat/test-chunked-array.R
##########
@@ -206,7 +206,7 @@ test_that("ChunkedArray supports empty arrays (ARROW-13761)", {
int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
uint64(), float32(), float64(), timestamp("ns"), binary(),
large_binary(), fixed_size_binary(32), date32(), date64(),
- decimal(4, 2), dictionary(), struct(x = int32())
+ decimal(4, 2), decimal128(4, 2), dictionary(), struct(x = int32())
Review comment:
I thought I'd keep all the tests for `decimal()` since we haven't deprecated it, but happy to remove them.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759559327
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range: 100")
Review comment:
Done. They are tested in the C++ code here: https://github.com/apache/arrow/blob/2baed02d259fe3534d20de14af6f7df48611b858/c_glib/test/test-decimal128-data-type.rb#L51
--
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] dragosmg commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981616398
Thanks @thisisnic and @paleolimbot for the comments. By the looks of it we have quite a few of the pieces for `decimal256()` in place.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r761088741
##########
File path: r/tests/testthat/test-dplyr-funcs-type.R
##########
@@ -208,32 +208,42 @@ test_that("type checks with is() giving Arrow types", {
Table$create(
i32 = Array$create(1, int32()),
dec = Array$create(pi)$cast(decimal(3, 2)),
+ dec128 = Array$create(pi)$cast(decimal128(3, 2)),
f64 = Array$create(1.1, float64()),
str = Array$create("a", arrow::string())
) %>% transmute(
i32_is_i32 = is(i32, int32()),
i32_is_dec = is(i32, decimal(3, 2)),
+ i32_is_dec128 = is(i32, decimal128(3, 2)),
i32_is_i64 = is(i32, float64()),
i32_is_str = is(i32, arrow::string()),
dec_is_i32 = is(dec, int32()),
dec_is_dec = is(dec, decimal(3, 2)),
+ dec_is_dec128 = is(dec, decimal128(3, 2)),
dec_is_i64 = is(dec, float64()),
dec_is_str = is(dec, arrow::string()),
+ dec128_is_i32 = is(dec128, int32()),
+ dec128_is_dec128 = is(dec128, decimal128(3, 2)),
+ dec128_is_i64 = is(dec128, float64()),
+ dec128_is_str = is(dec128, arrow::string()),
f64_is_i32 = is(f64, int32()),
f64_is_dec = is(f64, decimal(3, 2)),
+ f64_is_dec128 = is(f64, decimal128(3, 2)),
f64_is_i64 = is(f64, float64()),
f64_is_str = is(f64, arrow::string()),
str_is_i32 = is(str, int32()),
str_is_dec = is(str, decimal(3, 2)),
+ str_is_dec128 = is(str, decimal128(3, 2)),
str_is_i64 = is(str, float64()),
str_is_str = is(str, arrow::string())
Review comment:
Done.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759547926
##########
File path: r/tests/testthat/test-chunked-array.R
##########
@@ -206,7 +206,7 @@ test_that("ChunkedArray supports empty arrays (ARROW-13761)", {
int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
uint64(), float32(), float64(), timestamp("ns"), binary(),
large_binary(), fixed_size_binary(32), date32(), date64(),
- decimal(4, 2), dictionary(), struct(x = int32())
+ decimal(4, 2), decimal128(4, 2), dictionary(), struct(x = int32())
Review comment:
Done
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759094997
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Same as for the other arg to `decimal128()`, this is a generic R error: `Error in decimal128(4) : argument "scale" is missing, with no default`. I merely duplicated the tests we had for `decimal()`. Given it's the rather generic R error, I'd say assertion on content might not be needed.
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot edited a comment on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-985537478
Benchmark runs are scheduled for baseline = ee32aeaaadf1f112a0e850a9050ab822ccfe5d07 and contender = 310fca93d7b44081418fafd5fd30ce9342839ee4. 310fca93d7b44081418fafd5fd30ce9342839ee4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e0a284ce1f0143d28b87a1eb1b19ba58...e10fc7e8835b43c89536d90a33ee8184/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e8289b13214b4f8fa9cf44d926e35819...08c88bd757ac45bd8792a9a6d1705474/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a45190b7a8534440b5dad714d8232e76...466dc75ac840447daf0096b8de1256dc/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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] jonkeane commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r758496846
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range: 100")
Review comment:
If the c++ errors are tested in the c++ code, we don't need to test them here as well.
##########
File path: r/tests/testthat/test-chunked-array.R
##########
@@ -206,7 +206,7 @@ test_that("ChunkedArray supports empty arrays (ARROW-13761)", {
int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
uint64(), float32(), float64(), timestamp("ns"), binary(),
large_binary(), fixed_size_binary(32), date32(), date64(),
- decimal(4, 2), dictionary(), struct(x = int32())
+ decimal(4, 2), decimal128(4, 2), dictionary(), struct(x = int32())
Review comment:
I think we can get rid of `decimal(...)` here since it's just a wrapper for `decimal128()`, right?
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
Review comment:
Do we want to also do something like `expect_identical(class(decimal(4, 2)), class(decimal128(4, 2))` that asserts that `decimal()` -> `decimal128()`
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
+ expect_error(decimal128(4, "two"), '"scale" must be an integer')
+ expect_error(decimal128(NA, 2), '"precision" must be an integer')
+ expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range: 0")
+ expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range: 100")
+ expect_error(decimal128(4, NA), '"scale" must be an integer')
+
+ expect_r6_class(decimal128(4, 2), "Decimal128Type")
Review comment:
This is super super minor, but maybe we should move this up above the `expect_errors()`. We aren't strict about this, but _for the most part_ the pattern we use is test the base cases, test edge cases, test errors (though I'm sure there are counter examples!)
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
Review comment:
Is this an error that the R package raises? If so, should we assert what it says?
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Is this an error that the R package raises? If so, should we assert what it says?
--
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] github-actions[bot] commented on pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#issuecomment-981520065
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759094997
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Same as for the other arg to `decimal128()`, this is a generic R error: `Error in decimal128(4) : argument "scale" is missing, with no default`. I merely duplicated the tests we had for `decimal()`.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759094997
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -395,6 +395,18 @@ test_that("decimal type and validation", {
expect_error(decimal(4, NA), '"scale" must be an integer')
expect_r6_class(decimal(4, 2), "Decimal128Type")
+
+ expect_error(decimal128())
+ expect_error(decimal128("four"), '"precision" must be an integer')
+ expect_error(decimal128(4))
Review comment:
Row 400 should be checked as that comes from the R code.
Rows 399 & 401: same as for the other arg to `decimal128()`, this is a generic R error: `Error in decimal128(4) : argument "scale" is missing, with no default`. I merely duplicated the tests we had for `decimal()`. Given it's the rather generic R error, I'd say assertion on content might not be needed.
I'd be in favour of keeping all of them.
--
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] dragosmg commented on a change in pull request #11790: ARROW-14843 [R] Implement `decimal128()` (to replace `decimal()`)
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11790:
URL: https://github.com/apache/arrow/pull/11790#discussion_r759149624
##########
File path: r/tests/testthat/test-chunked-array.R
##########
@@ -206,7 +206,7 @@ test_that("ChunkedArray supports empty arrays (ARROW-13761)", {
int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
uint64(), float32(), float64(), timestamp("ns"), binary(),
large_binary(), fixed_size_binary(32), date32(), date64(),
- decimal(4, 2), dictionary(), struct(x = int32())
+ decimal(4, 2), decimal128(4, 2), dictionary(), struct(x = int32())
Review comment:
I thought I'd keep all the tests for `decimal()` since we haven't deprecated it, but happy to remove them / some of them.
--
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