New StringArray Iterator API should not include Option

[ehuss]

In #1241, the StringArray iterator was changed from returning Option<&str> to Result<Option<&str>>. I think it should instead have been changed to Result<&str>. The inner Option is unnecessary because it should only ever be None if there is a bug in the iterator implementation. This results in an unnecessary unwrap for the caller. I think the iterator should be:

    type Item = Result<&'a str, Error>;
    fn next(&mut self) -> Option<Result<&'a str, Error>> {
        self.range.next().map(|i| self.arr.get(i).transpose().unwrap())
    }

cc @weihanglo @DanielEScherzer

[ehuss]

Additionally, the StringArray::iter docs seem to be outdated now, as it says it just yields Option<&str>.

[DanielEScherzer]

In #1241, the StringArray iterator was changed from returning Option<&str> to Result<Option<&str>>. I think it should instead have been changed to Result<&str>. The inner Option is unnecessary because it should only ever be None if there is a bug in the iterator implementation. This results in an unnecessary unwrap for the caller. I think the iterator should be:

type Item = Result<&'a str, Error>;
fn next(&mut self) -> Option<Result<&'a str, Error>> {
    self.range.next().map(|i| self.arr.get(i).transpose().unwrap())
}

cc @weihanglo @DanielEScherzer

I generally prefer not to use unwrap and to let the caller see the None rather than a panic - what about converting None to an Err() value?

Additionally, the StringArray::iter docs seem to be outdated now, as it says it just yields Option<&str>.

Sorry I missed that, I was checking the documentation of the updated methods but not those around them. I sent #1265 (not linked as Closes or Fixes since there is still the question about unwrap vs converting to an error)

[ehuss]

Generally Err is used for expected errors. In this case, if get returns None, then it indicates a bug in the code which is what panic is often used for. In this case, the system is in an unknown state (this should never happen), so it could be unsafe to continue processing. It's also an expected impossible state, so adding the complexity of a new Error type seems overkill here.

[DanielEScherzer]

Okay, makes sense - I closed #1265, new patch coming momentarily