You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "amoeba (via GitHub)" <gi...@apache.org> on 2025/01/24 16:15:11 UTC
[PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
amoeba opened a new pull request, #45346:
URL: https://github.com/apache/arrow/pull/45346
### Rationale for this change
Fixes https://github.com/apache/arrow/issues/45300.
### What changes are included in this PR?
Updated check in `arrow_attributes` in metadata.R.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Users may be depending on data.tables round-tripping so this may be considered a very minor breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2612927986
@github-actions crossbow submit test-r-extra-packages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613185925
> Thanks! I'll take a look at the setDT idea later today. I'm also considering whether it would work to just drop the `.internal.selfref` attribute.
I thought we already did--it's an externalptr right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "ben-schwen (via GitHub)" <gi...@apache.org>.
ben-schwen commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613683879
> > "Safe option" would be not to drop the `data.table` class but only drop the `.internal.selfref` attribute, so `data.table` will detect that and allocate the right memory.
>
> I thought that's what we're already doing, so maybe I misunderstand the original report.
I consider the initial report as WAI, although our warning could be better since we also warn about the serializing case when `saveRDS` and subsequent `readRDS`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613382214
You're both right about the behavior. The `.internal.selfref` attribute is removed and data.table self-heals like @jonkeane said,
```r
> library(data.table)
> dt_in <- data.table(x = 1:10)
> attributes(dt_in)
$names
[1] "x"
$row.names
[1] 1 2 3 4 5 6 7 8 9 10
$class
[1] "data.table" "data.frame"
$.internal.selfref
<pointer: 0x13b821ae0>
> arrow::write_parquet(dt_in, "test.parquet")
> dt_out <- read_parquet("test.parquet")
> attributes(dt_out)
$names
[1] "x"
$row.names
[1] 1 2 3 4 5 6 7 8 9 10
$class
[1] "data.table" "data.frame"
> setDT(dt_out)
> attributes(dt_out)
$names
[1] "x"
$row.names
[1] 1 2 3 4 5 6 7 8 9 10
$class
[1] "data.table" "data.frame"
$.internal.selfref
<pointer: 0x13b821ae0>
```
It doesn't make sense to me how the `.internal.selfref` pointer is the same on both sides 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba closed pull request #45346: GH-45300: [R] Remove data.table from class attribute in metadata
URL: https://github.com/apache/arrow/pull/45346
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2612933627
I haven't read the SO post yet, but FWIW, we added some extra tests confirming that generally speaking data.table objects do roundtrip. https://github.com/apache/arrow/pull/43634 has some more details, but IIUC the pointed not being roundtripped is _ok_ and data.table should recreate it when it needs to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "TysonStanley (via GitHub)" <gi...@apache.org>.
TysonStanley commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613508193
Yeah, this is even true if you don't attach `data.table` but interestingly even with the same `.internal.selfref` modifying `dt_in` does not result in changes in `dt_out` so they are seen as different objects with the same `.internal.selfref`.
``` r
dt_in <- data.table::data.table(x = 1:10)
attributes(dt_in)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3 4 5 6 7 8 9 10
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x141813ee0>
arrow::write_parquet(dt_in, "test.parquet")
dt_out <- arrow::read_parquet("test.parquet")
attributes(dt_out)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3 4 5 6 7 8 9 10
#>
#> $class
#> [1] "data.table" "data.frame"
data.table::setDT(dt_out)
attributes(dt_out)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3 4 5 6 7 8 9 10
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x141813ee0>
dt_in[, y := 10:1]
attributes(dt_in)
#> $names
#> [1] "x" "y"
#>
#> $row.names
#> [1] 1 2 3 4 5 6 7 8 9 10
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x141813ee0>
attributes(dt_out)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3 4 5 6 7 8 9 10
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x141813ee0>
print(dt_in)
#> x y
#> <int> <int>
#> 1: 1 10
#> 2: 2 9
#> 3: 3 8
#> 4: 4 7
#> 5: 5 6
#> 6: 6 5
#> 7: 7 4
#> 8: 8 3
#> 9: 9 2
#> 10: 10 1
print(dt_out)
#> x
#> <int>
#> 1: 1
#> 2: 2
#> 3: 3
#> 4: 4
#> 5: 5
#> 6: 6
#> 7: 7
#> 8: 8
#> 9: 9
#> 10: 10
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613008898
I guess an alternative to dropping the data.table class, if we wanted to round-trip data.table objects, would be when applying R metadata on read, if class contains data.table, call `setDT()` on the data frame?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "ben-schwen (via GitHub)" <gi...@apache.org>.
ben-schwen commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613640506
> Yeah, this is even true if you don't attach `data.table` but interestingly even with the same `.internal.selfref` modifying `dt_in` does not result in changes in `dt_out` so they are seen as different objects with the same `.internal.selfref`.
Nothing unexpected. It will even create the same `.internal.selfref` for different objects.
``` r
library(data.table)
dt = data.table(x=1:3)
attributes(dt)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x5640ce7bc310>
dt2 = data.table(x=1:3)
attributes(dt2)
#> $names
#> [1] "x"
#>
#> $row.names
#> [1] 1 2 3
#>
#> $class
#> [1] "data.table" "data.frame"
#>
#> $.internal.selfref
#> <pointer: 0x5640ce7bc310>
```
But you can always check that they are not the same by checking `address(dt) != address(dt2)`. "Safe option" would be not to drop the `data.table` class but only drop the `.internal.selfref` attribute, so `data.table` will detect that and allocate the right memory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613662934
> "Safe option" would be not to drop the `data.table` class but only drop the `.internal.selfref` attribute, so `data.table` will detect that and allocate the right memory.
I thought that's what we're already doing, so maybe I misunderstand the original report.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613140247
Thanks! I'll take a look at the setDT idea later today. I'm also considering whether it would work to just drop the `.internal.selfref` attribute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2612901205
:warning: GitHub issue #45300 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2612932377
Revision: 9827ff12e12fc2df62da0e1bff06a380b7955edc
Submitted crossbow builds: [ursacomputing/crossbow @ actions-e869f440e0](https://github.com/ursacomputing/crossbow/branches/all?query=actions-e869f440e0)
|Task|Status|
|----|------|
|test-r-extra-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/12953594570/job/36133617891)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613713197
We can silence the warning if we do what @nealrichardson suggested in https://github.com/apache/arrow/pull/45346#issuecomment-2613008898 though I'm not sure whether we want to or not. I pushed up https://github.com/apache/arrow/pull/45346/commits/459bde8b4bac76d31e70b8fcf45088ef1d99ea8b for discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2614454649
Thanks for all the info @TysonStanley and @ben-schwen
> "Safe option" would be not to drop the data.table class but only drop the `.internal.selfref` attribute, so data.table will detect that and allocate the right memory. ... I consider the initial report as WAI, although our warning could be better since we also warn about the serializing case when `saveRDS` and subsequent `readRDS`.
_nods_ yeah, this sounds good to me. I'm happy to help with a PR for improving that warning in `data.table`. It makes a lot of sense for it to be there since it's about serializing those objects in any fashion, not just with Arrow/Parquet, but I wonder if there is anything that we should add to our documentation that calls this out as well. We could add something to https://arrow.apache.org/docs/r/articles/metadata.html#r-object-attributes which calls this case out as one of the cases where we can't preserve a portion of metadata, but the way that `data.table` works that's working as expected and self heals, etc.
> We can silence the warning if we do what @nealrichardson suggested in https://github.com/apache/arrow/pull/45346#issuecomment-2613008898 though I'm not sure whether we want to or not. I pushed up https://github.com/apache/arrow/commit/459bde8b4bac76d31e70b8fcf45088ef1d99ea8b for discussion.
Thanks for pushing this up for discussion (and extra thanks for jumping on this overall), IMHO we probably don't want to do this. If I am understanding correctly, there will still be a (small) performance hit when the memory is allocated and we should defer to `data.table`'s self healing here since it knows best when and how (and can message appropriately that there might be a small performance degradation that is coming from the fact that any serialization happened).
It sounds like we will likely close this PR, but I can go back to the issue and follow up with a TL;DR there explaining what we learned here (and closing that too since it's working as intended). I'm a little surprised in the original issue they mention this (seeming to) work in older versions of Arrow. There's no way it _actually did_ serialize the pointer and bring it back faithfully, but maybe something in #41969 changed the serialization slightly such that `data.table` is now better able to recognize and warn about the need to self-heal?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45300: [R] Remove data.table from class attribute in metadata [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #45346:
URL: https://github.com/apache/arrow/pull/45346#issuecomment-2613257866
> so data.table was confused to see an object that declared itself to be a data.table but that didn't have the pointer attached.
IIUC data.table self-heals in situations like these. You might have a performance hit the first time you do something when it recreates the pointer, but that's expected with any serialization of data.table (it's not unique to arrow or parquet). @TysonStanley might be able to correct me if I'm off here (or advise if there's a better way for this to happen)
--
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