Add skipLines in FileIO.ReadableFile.readFullyAsUTF8String

[gudfhr95]

Fixes #17990

Changes

• Add skipLines in FileIO.ReadableFile.readFullyAsUTF8String
• Add skipLines in FileIO.ReadableFile.readFullyAsBytes
• Refactored FileIOTest to Enclosed.class to add parameterized test

Related PR

• Add skip header lines in TextIO #28502

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

[liferoad]

R: @shunping-google

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

[gudfhr95]

Hi, @damondouglas.
I finished implementing what you suggested.
I'm looking forward for your review.
Thank you.

[damccorm]

@damondouglas or @shunping-google could you please take a look at this one?

[ghost]

Hi gudfhr95, I am reviewing your change today and will get back to you shortly. Sorry about the delay.

[ghost]

gudfhr95,

Thanks again for submitting your PRs for review! I spent some time discussing with the previous reviewer (@damondouglas) about this PR (#28728) offline and below is our opinion.

While there are merits in your changes, we also see certain problems that we will have to address:

• It modifies the signature of the public function readFullyAsBytes(), which will inevitably break the backward compatibility of existing code. This is something we want to avoid.
• It may not work on large files since it has to put all file content in the memory.
• It only considers "\n" as the line delimiter, while there could be more out there.

In fact, I think FileIO is not the best place for this change. In the design of Beam IO, FileIO should be mainly focusing on getting a readable/writable channel ready. It should not have any assumption on what's inside the file. If you put the header skipping feature in FileIO, you are assuming it is a text-like file that has line breaks.

We prefer putting the change in a class derived from FileIO. Just like your original PR (#28502), TextIO will be the best choice as it is for text files and it has already handled different types of delimiters. However, the original PR also introduced some breaking changes to the public functions. Moreover, it will not scale well because it needs to disable isSplittable() when the header skipping feature is used.

In summary, we see problems in both PRs and it will be great if we can

1. implement the changes in TextIO,

2. keep the capability of parallel processing for a large file, and

3. make sure there are no breaking changes (instead of changing the signature of a public function, try to overload it).

There is another PR (#29202) submitted recently on implementing this feature. You can refer to it for some ideas.

Thanks,

Shunping

[gudfhr95]

Hi, since #29202 was merged.
There are no reason to continue this PR.
Thanks for your reviews.