Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
conbench-apache-arrow[bot] commented on PR #46720: URL: https://github.com/apache/arrow/pull/46720#issuecomment-2978495786 After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 202787f663ca0f9c0334591a91d34073a0b76765. There were 115 benchmark results with an error: - Commit Run on `arm64-t4g-2xlarge-linux` at [2025-06-16 16:31:03Z](https://conbench.ursa.dev/compare/runs/35394c0f16fd4fccad5ced16bcfdb421...6ae4261c2e994c6d805a92d3aa2eaa96/) - [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06850582f4c67cb88000f7e0ae6c3c50) - [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0685058e475d71518000f01f171d4c1d) - and 113 more (see the report linked below) There were no benchmark performance regressions. 🎉 The [full Conbench report](https://github.com/apache/arrow/runs/44216748058) has more details. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic merged PR #46720: URL: https://github.com/apache/arrow/pull/46720 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2150313048
##
r/R/enums.R:
##
@@ -80,17 +80,23 @@ Type <- enum("Type::type",
LARGE_BINARY = 35L,
LARGE_LIST = 36L,
INTERVAL_MONTH_DAY_NANO = 37L,
- RUN_END_ENCODED = 38L
+ RUN_END_ENCODED = 38L,
+ STRING_VIEW = 39L,
+ BINARY_VIEW = 40L,
+ LIST_VIEW = 41L,
+ LARGE_LIST_VIEW = 42L,
Review Comment:
I don't really know what these enums do, haven't traced it through, but my
guess would be that they don't matter as we haven't implemented them in R?
Hmm, in which case, maybe I remove? (Can do in a follow-up)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2150307214
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
Done - #46825
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2150313048
##
r/R/enums.R:
##
@@ -80,17 +80,23 @@ Type <- enum("Type::type",
LARGE_BINARY = 35L,
LARGE_LIST = 36L,
INTERVAL_MONTH_DAY_NANO = 37L,
- RUN_END_ENCODED = 38L
+ RUN_END_ENCODED = 38L,
+ STRING_VIEW = 39L,
+ BINARY_VIEW = 40L,
+ LIST_VIEW = 41L,
+ LARGE_LIST_VIEW = 42L,
Review Comment:
I don't really know what these enums do, haven't traced it through, but my
guess would be that they don't matter as we haven't implemented them in R?
Hmm, in which case, maybe I remove?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
jonkeane commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2150201021
##
r/R/enums.R:
##
@@ -80,17 +80,23 @@ Type <- enum("Type::type",
LARGE_BINARY = 35L,
LARGE_LIST = 36L,
INTERVAL_MONTH_DAY_NANO = 37L,
- RUN_END_ENCODED = 38L
+ RUN_END_ENCODED = 38L,
+ STRING_VIEW = 39L,
+ BINARY_VIEW = 40L,
+ LIST_VIEW = 41L,
+ LARGE_LIST_VIEW = 42L,
Review Comment:
I'm a little surprised there weren't / aren't tests that caught this? But
maybe I don't totally grok what's up with these
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
jonkeane commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2150197656
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
It would mean being able to delete some code in the R package, which is nice
— let's make an issue but agreed we don't need to tackle it 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2149645686
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
There were tests added
[here](https://github.com/apache/arrow/pull/43957/files#diff-1bdade8037e08dc30047ce7d8218c63e39f908d416be1b2b1c586dcd410dfc1aR2226-R2240)
so we can get rid of our own tests for this, but the `decimal()` C++ function
was [deprecated in favour of
`smallest_decimal()`](https://github.com/apache/arrow/pull/43957/files#diff-79bec37f77fa174c78a55b17c75d71fe6b566b04e1fc82385f00b76ac908d63eR532)
so I'll need to swap that out too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on PR #46720: URL: https://github.com/apache/arrow/pull/46720#issuecomment-2976089619 Cheers for the review @jonkeane. I've rebased to see if that fixes the linter error, but will run locally anyway to make sure. Any other changes to make before we merge? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2149657447
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
OK, we don't actually use the C++ `decimal()` function anyway, so no changes
needed. We could look to use `smallest_decimal()` instead of working out which
decimal it should be ourselves in the R code, but I don't think the time it
would take to make this change would be worth the effort, though can open a new
ticket if you disagree.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2149645686
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
There were tests added
[here](https://github.com/apache/arrow/pull/43957/files#diff-1bdade8037e08dc30047ce7d8218c63e39f908d416be1b2b1c586dcd410dfc1aR2226-R2240)
so we can get rid of our own tests for this, but the `decimal()` C++ function
was deprecated in favour of `smallest_decimal()` so I'll need to swap that out
too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2145920066
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
Oh wait, I confused the JIRA and GitHub numbers. Will double check the C++
code to double check though.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2145918196
##
r/R/enums.R:
##
@@ -80,17 +80,23 @@ Type <- enum("Type::type",
LARGE_BINARY = 35L,
LARGE_LIST = 36L,
INTERVAL_MONTH_DAY_NANO = 37L,
- RUN_END_ENCODED = 38L
+ RUN_END_ENCODED = 38L,
+ STRING_VIEW = 39L,
+ BINARY_VIEW = 40L,
+ LIST_VIEW = 41L,
+ LARGE_LIST_VIEW = 42L,
Review Comment:
Yeah, went and looked the decimal ones up in the C++ and found a few more
missing too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
jonkeane commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2145868597
##
r/tests/testthat/test-data-type.R:
##
@@ -497,10 +496,6 @@ test_that("decimal type and validation", {
expect_error(decimal128(4, NA), "`scale` must be an integer")
expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2")
expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2")
- # TODO remove precision range tests below once functionality is tested in
C++ (ARROW-15162)
- expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1,
38]: 0", fixed = TRUE)
- expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range
[1, 38]: 100", fixed = TRUE)
-
Review Comment:
Thanks for this cleanup, though am I misunderstanding that
https://github.com/apache/arrow/issues/30667 is still open? Or maybe this was
actually implemented somewhere and we should close that issue?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
jonkeane commented on code in PR #46720:
URL: https://github.com/apache/arrow/pull/46720#discussion_r2145865021
##
r/R/enums.R:
##
@@ -80,17 +80,23 @@ Type <- enum("Type::type",
LARGE_BINARY = 35L,
LARGE_LIST = 36L,
INTERVAL_MONTH_DAY_NANO = 37L,
- RUN_END_ENCODED = 38L
+ RUN_END_ENCODED = 38L,
+ STRING_VIEW = 39L,
+ BINARY_VIEW = 40L,
+ LIST_VIEW = 41L,
+ LARGE_LIST_VIEW = 42L,
Review Comment:
Were we just missing these?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46719: [R] Add 32 and 64 bit Decimal types [arrow]
thisisnic commented on code in PR #46720: URL: https://github.com/apache/arrow/pull/46720#discussion_r2145340640 ## r/vignettes/data_types.Rmd: ## @@ -96,26 +122,38 @@ Note that the Arrow specification also permits half-precision (16 bit) floating ## Fixed point decimal types -Arrow also contains `decimal()` data types, in which numeric values are specified in decimal format rather than binary. Decimals in Arrow come in two varieties, a 128 bit version and a 256 bit version, but in most cases users should be able to use the more general `decimal()` data type function rather than the specific `decimal128()` and `decimal256()` functions. +Arrow also contains `decimal()` data types, in which numeric values are specified in decimal format rather than binary. +Decimals in Arrow come in two varieties, a 128 bit version and a 256 bit version, but in most cases users should be able +to use the more general `decimal()` data type function rather than the specific `decimal32()`, `decimal64()`, `decimal128()`, Review Comment: I split the text up in this vignette to make it easier to edit and track edits in future, but the only actual changes I made were to this line, where I added the new decimal types in. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
