[C++] ReadAt/WriteAt are inconsistent with moving the files position

[asfimport]

Right now, there is inconsistent behaviour regarding moving the files position pointer after calling ReadAt or WriteAt. For example, the default implementation of ReadAt seeks to the desired offset and calls Read which moves the position pointer. MemoryMappedFile::ReadAt, however, doesn't change the position. WriteableFile::WriteAt seem to move the position in the current implementation, but there is no docstring which prescribes this behaviour.

Antoine suggested that *At methods shouldn't touch the position and it makes more sense, IMHO. The change isn't huge and doesn't seem to break anything internally, but it might break the existing user code.

Reporter: Dimitri Vorona / @alendit
Assignee: Antoine Pitrou / @pitrou

PRs and other links:

• GitHub Pull Request #4417

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

[asfimport]

Antoine Pitrou / @pitrou:
Unfortunately, things are bit more complicated as in some cases, ReadAt / WriteAt are forced to update the file position anyway (on Windows, see implementation of FileRead in io-util.cc).

[asfimport]

Dimitri Vorona / @alendit:
Ok, then we should at least be consistent across implementation, i.e. advance the position in MemoryMappedFile::ReadAt, right?

[asfimport]

Antoine Pitrou / @pitrou:
In that case it would mean only advance the position if we're on Windows :) I don't think there's a nice way out of this. If we want to keep the parallelization benefits of ReadAt and WriteAt, we'll need to live with the platform specifics, I think. @wesm

[asfimport]

Wes McKinney / @wesm:
I agree that we may in general have to live with the platform specific behavior. When there is a choice (rather than being a platform specific detail), I would support having ReadAt/WriteAt set the position to the end of the read segment (or the end of the file, as the case may be). Does that sound like a reasonable scope for this JIRA?

[asfimport]

Wes McKinney / @wesm:
Moving this off 0.10.0 for now. I will focus on getting ARROW-2701 merged

[asfimport]

Wes McKinney / @wesm:
Do we want to make any changes here for 0.11?

[asfimport]

Wes McKinney / @wesm:
Bumping this issue. It seems this is mostly a documentation issue, but if we want to make MemoryMappedFile::WriteAt/ReadAt behave a certain way (e.g advancing the file position), feel free to propose changes

[asfimport]

Antoine Pitrou / @pitrou:
I see two other ways around this:

1. As soon as ReadAt or WriteAt is called, change the internal file state so that any implicitly-positioning operation (such as Read, Write or Tell) fails until Seek is called first.

or 2) Have an internal "positioning" lock that ensures that we can have several ReadAt or WriteAt calls simultaneously, but that implicitly positioning operations wait for the last *At call to end and to restore the file pointer.

I'm not sure how easy #2 is, but should be doable.

[asfimport]

Antoine Pitrou / @pitrou:
I'm leaning towards #1, because it helps expose bugs.

As a sidenote, Tensorflow's RandomAccessFile only supports ReadAt-like operation, not implicit positioning with Seek() or Tell(). See https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/platform/file_system.h#L231

[asfimport]

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