Skip to content

ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar#9345

Closed
nealrichardson wants to merge 6 commits intoapache:masterfrom
nealrichardson:arrow-datum
Closed

ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar#9345
nealrichardson wants to merge 6 commits intoapache:masterfrom
nealrichardson:arrow-datum

Conversation

@nealrichardson
Copy link
Member

The impetus for this change was to get S3 Ops method dispatch to work for Array == Scalar. This is tested in the third commit.

@github-actions
Copy link

r/R/table.R Outdated
Copy link
Member

@ianmcook ianmcook Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up moving them to ArrowTabular and left stop(TODO) messages in the missing RecordBatch methods.

@ianmcook
Copy link
Member

ianmcook commented Jan 29, 2021

@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.

@nealrichardson
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants