Skip to content

ARROW-14989: [R] Update num_rows methods to output doubles not integers to prevent integer overflow - #13482

Closed
thisisnic wants to merge 12 commits into
apache:masterfrom
thisisnic:ARROW-14989_num_rows_double
Closed

ARROW-14989: [R] Update num_rows methods to output doubles not integers to prevent integer overflow#13482
thisisnic wants to merge 12 commits into
apache:masterfrom
thisisnic:ARROW-14989_num_rows_double

Conversation

@thisisnic

@thisisnicthisisnic commented Jun 30, 2022

Copy link
Copy Markdown
Member

This PR enables num_rows() methods to be called on Table and RecordBatch objects without integer overflow when the value of num_rows() is higher than .Machine$integer.max. I originally wrote some tests but they take ages to run and crashed on CI anyway so I removed them, but they can be seen in https://github.com/apache/arrow/pull/13482/commits/e7cf8a66beab6d1b7d85304362086b6205a31279/.

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnicthisisnic changed the title ARROW-14989: [R] Update num_rows methods to output doubles not integers to prevent integer overflow [WIP]ARROW-14989: [R] Update num_rows methods to output doubles not integers to prevent integer overflowJun 30, 2022
@thisisnic
thisisnic marked this pull request as ready for review June 30, 2022 19:17

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

These changes should get it to work...the explicit keyword disables C++'s automatic generation of implicit construction via assignment (i.e., r_vec_size some_variable = (R_x_len_t) 123;), which is what happens when you return something (maybe I'm getting the details slightly wrong here but that's how I've internalized it...the documentation on this is not awesome: https://en.cppreference.com/w/cpp/language/explicit ).

It would be a little more slick to define the proper converting constructor so that you can just return some_int64; but I don't know the exact incantation to make that work.

Comment threadr/src/recordbatch.cpp Outdated
Comment threadr/src/table.cpp Outdated
thisisnicand others added 2 commits July 4, 2022 13:10
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 41f8bdf and contender = 7124baf. 7124baf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.15% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7124bafb ec2-t3-xlarge-us-east-2
[Finished] 7124bafb test-mac-arm
[Finished] 7124bafb ursa-i9-9960x
[Finished] 7124bafb ursa-thinkcentre-m75q
[Finished] 41f8bdfe ec2-t3-xlarge-us-east-2
[Failed] 41f8bdfe test-mac-arm
[Finished] 41f8bdfe ursa-i9-9960x
[Finished] 41f8bdfe ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@nealrichardson

Copy link
Copy Markdown
Member

I think there's at least one more row count that needs this attention: https://github.com/apache/arrow/blob/master/r/src/dataset.cpp#L514

drin pushed a commit to drin/arrow that referenced this pull request Jul 5, 2022
…rs to prevent integer overflow
This PR enables `num_rows()` methods to be called on `Table` and `RecordBatch` objects without integer overflow when the value of `num_rows()` is higher than `.Machine$integer.max`. I originally wrote some tests but they take ages to run and crashed on CI anyway so I removed them, but they can be seen in https://github.com/apache/arrow/pull/13482/commits/e7cf8a66beab6d1b7d85304362086b6205a31279/.
Closesapache#13482 from thisisnic/ARROW-14989_num_rows_double
Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
nealrichardson pushed a commit that referenced this pull request Jul 14, 2022
…n large datasets (#13514)
Follow up to #13482 after this one was missed.
Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
kou pushed a commit that referenced this pull request Feb 20, 2023
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.

4 participants

@thisisnic@ursabot@nealrichardson@paleolimbot