[C++] RecordBatchReader should support STL-like iteration

[asfimport]

Our custom Iterator<T> has support for STL-like iteration (allowing natural for loops), but RecordBatchReader doesn't. Adding STL-like iteration would be a significant quality of life improvement for developers

Reporter: Antoine Pitrou / @pitrou
Assignee: Dhruv Vats / @dhruv9vats

PRs and other links:

• GitHub Pull Request #11946

_{Note: This issue was originally created as ARROW-13663. Please see the migration documentation for further details.}

[asfimport]

Dhruv Vats / @dhruv9vats:
May I give this is a shot?

[asfimport]

Neal Richardson / @nealrichardson:
Go for it! If no one has assigned themselves to the issue, it's generally fair game. I've assigned it to you now.

[asfimport]

Dhruv Vats / @dhruv9vats:
From what I understand, to support STL-like iteration, the following operators will have to be overloaded:

++ (increment) (also decrement?)

* (dereference)

!= (inequality)

== (equality)

right?

 

Also, I'm trying to find my way through the code; in this context, could some basic guidelines be provided? Like how should this functionality be implemented? Whether to use a nested class like in the case of {}Iterator<T>{}, or some other way?

 

Sorry if I'm overlooking something obvious.

[asfimport]

Antoine Pitrou / @pitrou:
Yes, a nested class sounds fine. You can just try to reuse what was done for Iterator<T>.

[asfimport]

Antoine Pitrou / @pitrou:
@dhruv9vats If you face any problem trying to implement this, please don't hesitate to mention!

[asfimport]

Dhruv Vats / @dhruv9vats:
Hey @pitrou,

Extremely sorry for the delay. Was caught up in unavoidable school work but am now actively working on this and will keep you posted. 

Apologies again.

[asfimport]

Antoine Pitrou / @pitrou:
It's fine, I just wanted to make sure you're still interested :-)

[asfimport]

Dhruv Vats / @dhruv9vats:
Because the RecordBatchReader is an abstract class, is there a straightforward way to define STL like iterators for it?

I tried using {}shared_ptr as given in Iterator<T>{}, but can't use them as the class is abstract. Does something like shared_from_this() have to be used? @pitrou  

[asfimport]

Antoine Pitrou / @pitrou:
Given the typical usage of STL iterators (i.e. mostly local and synchronous), I think the iterator can simply be non-owning, so you only need a raw pointer to the {}RecordBatchReader{}.  @bkietz  What do you think?

[asfimport]

Dhruv Vats / @dhruv9vats:
Sorry for all the delay. Written theory exams should not exist anymore.

Does the basic structure in the PR look correct? 

[asfimport]

Antoine Pitrou / @pitrou:
Yes, the basic structure looks ok. You'll probably need to add some tests before I review it.

[asfimport]

Antoine Pitrou / @pitrou:
Issue resolved by pull request 11946
#11946