Skip to content

ARROW-13904: [R] Implement ModeOptions - #11092

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-13904_modeoptions
Closed

ARROW-13904: [R] Implement ModeOptions#11092
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-13904_modeoptions

Conversation

@thisisnic

Copy link
Copy Markdown
Member

No description provided.

@github-actions

Copy link
Copy Markdown

@pitroupitrou left a comment

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.

A quick review on the C++ part. I won't comment on the R tests.

Comment threadr/src/compute.cpp Outdated
using Options = arrow::compute::ModeOptions;
auto out = std::make_shared<Options>(Options::Defaults());
if (!Rf_isNull(options["n"])) {
out->n = cpp11::as_cpp<int>(options["n"]);

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.

ModeOptions::n is an int64_t, so perhaps use cpp11::as_cpp<int64_t>?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Cheers for spotting that, will update them both!

Comment threadr/src/compute.cpp Outdated
out->n = cpp11::as_cpp<int>(options["n"]);
}
if (!Rf_isNull(options["min_count"])) {
out->min_count = cpp11::as_cpp<int>(options["min_count"]);

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.

For the same reason, perhaps cpp11::as_cpp<uint32_t>.

ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
Closesapache#11092 from thisisnic/ARROW-13904_modeoptions
Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
@asfimportasfimport mentioned this pull request Sep 13, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thisisnic@pitrou@nealrichardson