ARROW-13834: [R][Documentation] Document the process of creating R bindings for compute kernels and rationale behind conventions

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[thisisnic]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13834

[github-actions]

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

[thisisnic]

This will need to be updated once #11904 is merged

[AlenkaF]

This is great work Nic! Not much to comment on, everything is very clear.

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

[thisisnic]

Also this PR fits nicely with the guide content. I would link to this document from two parts of the guide (bindings and the tutorial). What do you think?

Sounds good to me!

[wjones127]

This looks great! My only two suggestions are:

1. Like Alenka said, explain why we are starting with tests. (I totally agree with this move, as someone who often will write tests at the end only to realize later they wouldn't have failed prior to my changes 🤦 .)
2. Consider combining steps 3 and 4 into one with subsections. For example, "Step 3: Map the R function to the C++ kernel", "Option a: use direct mapping if there are no options class", "Option b: add explicit implementation to nse_funcs"

[paleolimbot]

Just a few comments...it looks great! One is rather amorphous (the term "binding") but that's just a search/replace if or when the term changes (so shouldn't affect this getting merged).

[paleolimbot]

This bit has me questioning the term "binding"...whereas str_detect() and match_substring_regex is a 1:1 link, many "bindings" implement a few Arrow compute functions linked together. I'm not sure that either terminology "we created an Arrow binding for str_detect()` or "we created an R binding for match_substring_regex" are correct.

[thisisnic]

I love this point, yeah, I see what you mean; this could cause confusion. What about now that I've rephrased it?

This is possible as a binding has been created between the call to the
stringr function str_detect() and the Arrow C++ code, here as a direct mapping
to match_substring_regex. You can see this for yourself by inspecting the
arrow data query object without retrieving the results via collect().

[paleolimbot]

Sorry I missed this last week! I like how you've rephrased it.

[jonkeane]

This is looking great. Thank you for the care and detail you've put into this. A few comments and a few minor linter clean ups.

I don't think there will be much, but we should make sure to come back and fix up anything if/when #11904 merges

[jonkeane]

This is fantastic, thanks for all the work. I’ve got two minor comments, you’re welcome to take or leave and then merge away!

[ursabot]

Benchmark runs are scheduled for baseline = f165370 and contender = c9588c5. c9588c5 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%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java