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/01/27 21:35:33 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
nealrichardson opened a new pull request #9345:
URL: https://github.com/apache/arrow/pull/9345
The impetus for this change was to get S3 Ops method dispatch to work for Array == Scalar. This is tested in the third commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#discussion_r566527458
##########
File path: r/R/table.R
##########
@@ -248,39 +203,9 @@ Table$create <- function(..., schema = NULL) {
}
}
-#' @export
-as.data.frame.Table <- function(x, row.names = NULL, optional = FALSE, ...) {
- df <- Table__to_dataframe(x, use_threads = option_use_threads())
- if (!is.null(r_metadata <- x$metadata$r)) {
- df <- apply_arrow_r_metadata(df, .unserialize_arrow_r_metadata(r_metadata))
- }
- df
-}
-
-#' @export
-as.list.Table <- as.list.RecordBatch
-
-#' @export
-row.names.Table <- row.names.RecordBatch
-
-#' @export
-dimnames.Table <- dimnames.RecordBatch
-
-#' @export
-dim.Table <- function(x) c(x$num_rows, x$num_columns)
-
#' @export
names.Table <- function(x) x$ColumnNames()
-#' @export
-`names<-.Table` <- function(x, value) x$RenameColumns(value)
-
-#' @export
-`[.Table` <- `[.RecordBatch`
-
-#' @export
-`[[.Table` <- `[[.RecordBatch`
-
#' @export
`[[<-.Table` <- function(x, i, value) {
Review comment:
Could you add a TODO/comment here describing why this has not yet been changed to a method of the base class `ArrowTabular`? And another one where `$<-.Table` is defined
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook edited a comment on pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
ianmcook edited a comment on pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#issuecomment-769519951
@nealrichardson could you please confirm that this is correct?
- `RecordBatch` and `Table` now inherit the synthetic class `ArrowTabular`
- `Array`, `ChunkedArray`, and `Scalar` now inherit the synthetic class `ArrowDatum`
- These synthetic classes do not exist in the C++ class hierarchy; they are used here because they simplify S3 method dispatch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#discussion_r566527458
##########
File path: r/R/table.R
##########
@@ -248,39 +203,9 @@ Table$create <- function(..., schema = NULL) {
}
}
-#' @export
-as.data.frame.Table <- function(x, row.names = NULL, optional = FALSE, ...) {
- df <- Table__to_dataframe(x, use_threads = option_use_threads())
- if (!is.null(r_metadata <- x$metadata$r)) {
- df <- apply_arrow_r_metadata(df, .unserialize_arrow_r_metadata(r_metadata))
- }
- df
-}
-
-#' @export
-as.list.Table <- as.list.RecordBatch
-
-#' @export
-row.names.Table <- row.names.RecordBatch
-
-#' @export
-dimnames.Table <- dimnames.RecordBatch
-
-#' @export
-dim.Table <- function(x) c(x$num_rows, x$num_columns)
-
#' @export
names.Table <- function(x) x$ColumnNames()
-#' @export
-`names<-.Table` <- function(x, value) x$RenameColumns(value)
-
-#' @export
-`[.Table` <- `[.RecordBatch`
-
-#' @export
-`[[.Table` <- `[[.RecordBatch`
-
#' @export
`[[<-.Table` <- function(x, i, value) {
Review comment:
Could you add a TODO/comment here describing why these have not yet been changed to be methods of the base class `ArrowTabular`? And another one where `$<-.Table` is defined
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9345:
URL: https://github.com/apache/arrow/pull/9345
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#discussion_r566967705
##########
File path: r/R/table.R
##########
@@ -248,39 +203,9 @@ Table$create <- function(..., schema = NULL) {
}
}
-#' @export
-as.data.frame.Table <- function(x, row.names = NULL, optional = FALSE, ...) {
- df <- Table__to_dataframe(x, use_threads = option_use_threads())
- if (!is.null(r_metadata <- x$metadata$r)) {
- df <- apply_arrow_r_metadata(df, .unserialize_arrow_r_metadata(r_metadata))
- }
- df
-}
-
-#' @export
-as.list.Table <- as.list.RecordBatch
-
-#' @export
-row.names.Table <- row.names.RecordBatch
-
-#' @export
-dimnames.Table <- dimnames.RecordBatch
-
-#' @export
-dim.Table <- function(x) c(x$num_rows, x$num_columns)
-
#' @export
names.Table <- function(x) x$ColumnNames()
-#' @export
-`names<-.Table` <- function(x, value) x$RenameColumns(value)
-
-#' @export
-`[.Table` <- `[.RecordBatch`
-
-#' @export
-`[[.Table` <- `[[.RecordBatch`
-
#' @export
`[[<-.Table` <- function(x, i, value) {
Review comment:
I ended up moving them to ArrowTabular and left `stop(TODO)` messages in the missing RecordBatch methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#issuecomment-768642391
https://issues.apache.org/jira/browse/ARROW-10089
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#issuecomment-769519951
@nealrichardson could you please confirm that this is correct?
- `RecordBatch` and `Table` now inherit the synthetic class `ArrowTabular`
- `Array`, `ChunkedArray`, and `Scalar` now inherit the synthetic class `ArrowDatum`
These synthetic classes do not exist in the C++ class hierarchy; they are used here because they simplify S3 method dispatch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#discussion_r566527458
##########
File path: r/R/table.R
##########
@@ -248,39 +203,9 @@ Table$create <- function(..., schema = NULL) {
}
}
-#' @export
-as.data.frame.Table <- function(x, row.names = NULL, optional = FALSE, ...) {
- df <- Table__to_dataframe(x, use_threads = option_use_threads())
- if (!is.null(r_metadata <- x$metadata$r)) {
- df <- apply_arrow_r_metadata(df, .unserialize_arrow_r_metadata(r_metadata))
- }
- df
-}
-
-#' @export
-as.list.Table <- as.list.RecordBatch
-
-#' @export
-row.names.Table <- row.names.RecordBatch
-
-#' @export
-dimnames.Table <- dimnames.RecordBatch
-
-#' @export
-dim.Table <- function(x) c(x$num_rows, x$num_columns)
-
#' @export
names.Table <- function(x) x$ColumnNames()
-#' @export
-`names<-.Table` <- function(x, value) x$RenameColumns(value)
-
-#' @export
-`[.Table` <- `[.RecordBatch`
-
-#' @export
-`[[.Table` <- `[[.RecordBatch`
-
#' @export
`[[<-.Table` <- function(x, i, value) {
Review comment:
Could you add a TODO/comment here describing why this have not yet been changed to be methods of the base class `ArrowTabular`? And another one where `$<-.Table` is defined
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9345: ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9345:
URL: https://github.com/apache/arrow/pull/9345#issuecomment-769931624
> @nealrichardson could you please confirm that this is correct?
>
> * `RecordBatch` and `Table` now inherit the synthetic class `ArrowTabular`
> * `Array`, `ChunkedArray`, and `Scalar` now inherit the synthetic class `ArrowDatum`
> * These synthetic classes do not exist in the C++ class hierarchy; they are used here because they simplify S3 method dispatch.
Correct, and I expanded on the comments about that in the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org