Skip to content

ARROW-3871: [R] Replace usages of C++ GetValuesSafely with new methods on ArrayData#3103

Closed
romainfrancois wants to merge 4 commits intoapache:masterfrom
romainfrancois:ARROW-3871/GetValuesSafely
Closed

ARROW-3871: [R] Replace usages of C++ GetValuesSafely with new methods on ArrayData#3103
romainfrancois wants to merge 4 commits intoapache:masterfrom
romainfrancois:ARROW-3871/GetValuesSafely

Conversation

@romainfrancois
Copy link
Copy Markdown
Contributor

To get rid of GetValuesSafely I've had to add variants of ArrayData.GetValues<> that take and offset as a parameter, this is useful e.g. for boolean arrays or string arrays.

// Access a buffer's data as a typed C pointer
template <typename T>
inline const T* GetValues(int i) const {
inline const T* GetValues(int i, int64_t offset_) const {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The foo_ naming is reserved for class members

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pitrou since this function can be used to access any of the buffers, the "GetValues" name could be misleading. Perhaps GetBufferAs<T>(i) or something?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps GetBufferValues? But that's starting to feel a bit verbose, especially the mutable version :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(personally I'm fine with GetValues, btw)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I think it's OK, especially as this is an internal developer API

// Access a buffer's data as a typed C pointer
template <typename T>
inline T* GetMutableValues(int i) {
inline T* GetMutableValues(int i, int64_t offset_) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

@wesm
Copy link
Copy Markdown
Member

wesm commented Dec 5, 2018

Now the offset parameter shadows the offset struct member. Maybe call it "absolute_offset" so it is clear that the ArrayData::offset member is not used?

Copy link
Copy Markdown
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1. thank you!

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3103 into master will increase coverage by 1.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3103      +/-   ##
==========================================
+ Coverage   87.04%   88.15%   +1.11%     
==========================================
  Files         492      434      -58     
  Lines       69104    65343    -3761     
==========================================
- Hits        60153    57605    -2548     
+ Misses       8850     7738    -1112     
+ Partials      101        0     -101
Impacted Files Coverage Δ
cpp/src/arrow/array.h 98.42% <100%> (+0.05%) ⬆️
cpp/src/plasma/thirdparty/ae/ae.c 72.03% <0%> (-0.95%) ⬇️
go/arrow/array/table.go
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/array/null.go
go/arrow/datatype_nested.go
go/arrow/array/string.go
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f4af26...bcb1082. Read the comment docs.

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.

4 participants