Add header removal for TextIO #29202

stankiewicz · 2023-10-30T18:37:23Z

Adds header removal for TextIO.Read / TextIO.ReadAll (with tests)

Related to ##28502 which may not work if number of lines is large.
Related to ##28728 which is not scalable if files are large.

github-actions · 2023-10-31T00:19:14Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

stankiewicz · 2023-10-31T14:59:02Z

Run Java_GCP_IO_Direct PreCommit

ghost · 2023-10-31T15:41:46Z

Hi @stankiewicz, thanks so much for contributing to Beam!

Per our discussion, I would recommend the following additional changes.

Overloading the public functions to avoid breaking changes to the existed apis. (I see you have already submitted a new commit for that. Thanks for doing it!)
Your current code works great when the number of header lines to skip is 1. Would you please also add the logic for the case when we need to skip multiple header lines? Our Python SDK has a similar code path that you may find it useful.

beam/sdks/python/apache_beam/io/textio.py

Line 113 in c7dc5bd

skip_header_lines=0,

Notice that in this scenario, each reader will have to skip the header lines and make sure their start position is larger than the last position of the header. This may hurt the performance. Therefore, we will keep all 3 code paths (number of header lines to skip = 0/None, 1, >1) and choose the most efficient one based on user input.
Last but not least, I suggest using the same parameter name as our Python API, i.e.skipHeaderLines.

Again, great work!

Regards,

Shunping

stankiewicz · 2023-11-03T14:06:23Z

Run Java_GCP_IO_Direct PreCommit

Abacn · 2023-11-03T14:44:14Z

CHANGES.md

@@ -76,6 +76,7 @@ should handle this. ([#25252](https://github.com/apache/beam/issues/25252)).
 * `upload_graph` as one of the Experiments options for DataflowRunner is no longer required when the graph is larger than 10MB for Java SDK ([PR#28621](https://github.com/apache/beam/pull/28621).
 * state amd side input cache has been enabled to a default of 100 MB. Use `--max_cache_memory_usage_mb=X` to provide cache size for the user state API and side inputs. (Python) ([#28770](https://github.com/apache/beam/issues/28770)).
 * Beam YAML stable release. Beam pipelines can now be written using YAML and leverage the Beam YAML framework which includes a preliminary set of IO's and turnkey transforms. More information can be found in the YAML root folder and in the [README](https://github.com/apache/beam/blob/master/sdks/python/apache_beam/yaml/README.md).
+* `TextIO` supports skipping header.


this would go to beam 2.53.0. Also, link to the GitHub issue

Please append the the original issue link to this line, like

TextIO supports skipping header. (Java)(#17990).

ghost · 2023-11-03T14:53:13Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/TextSource.java

+    private void skipHeader(int headerLines, boolean skipFirstLine) throws IOException {
+      if (headerLines == 1) {
+        readNextRecord();
+      } else if (headerLines > 1) {


Could you also add a test to cover the skipping of multiple header lines?

ghost · 2023-11-03T15:07:04Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/TextSource.java

-          ((SeekableByteChannel) channel).position(requiredPosition);
-          startOfNextRecord = requiredPosition;
+          skipHeader(skipHeaderLines, false);
+          if (requiredPosition > startOfNextRecord) {


I am wondering what will happen if multiple readers having requiredPosition <= startOfNextRecord here.
In this case, more than one readers are assigned to a start position that falls in the header lines (assuming there are multiple long header lines). Not sure if the current code is handling this.

nvm. In this case, among those readers, only one will have an end position outside of the header lines, and will continue to read. So this should not be an issue then.

stankiewicz · 2023-11-03T18:01:48Z

Run Java_GCP_IO_Direct PreCommit

stankiewicz · 2023-11-03T19:11:30Z

Run Java PreCommit

stankiewicz · 2023-11-06T08:23:29Z

@shunping-google this is ready for review. after many retries of flaky tests, all green.

ghost

LGTM! Thanks again for contributing to Beam.

Abacn

Thanks for resolving this long standing feature request!

Please resolve the merge conflict then we can get this in

(not for this PR) Also this remind me that we should better check the correctness of our wordcount precommit test, not only run them

Abacn · 2023-11-08T14:58:48Z

testOrderedWithinKey (org.apache.beam.sdk.io.gcp.spanner.changestreams.it.SpannerChangeStreamOrderedWithinKeyIT) failed unrelated flaky test. merging for now

add header removal

1d917d8

github-actions bot added the java label Oct 30, 2023

stankiewicz added 5 commits October 30, 2023 19:19

spotless

e3e547d

spotless

63f3dec

tests

c4f3d6c

tests

79015dc

tests

ae385f9

changes

85034e4

ghost mentioned this pull request Oct 31, 2023

Add skipLines in FileIO.ReadableFile.readFullyAsUTF8String #28728

Closed

3 tasks

avoiding breaking change

a647724

stankiewicz added 9 commits November 2, 2023 22:41

add multiple header lines removal

2153404

checkstyle

20aeb2c

broken tests

d4755d6

Merge branch 'master' into textio-header-removal

c29672c

broken tests

d9499b2

broken tests

6ef369b

reverting breaking changes

e8db3e7

reverting breaking changes

fbe5f5f

spotless

f3b61fe

Abacn reviewed Nov 3, 2023

View reviewed changes

ghost reviewed Nov 3, 2023

View reviewed changes

stankiewicz added 3 commits November 3, 2023 15:40

added new changes, fixed edge cases, added new test

7903169

added new changes

5f99fbf

lint

62baa30

ghost approved these changes Nov 6, 2023

View reviewed changes

Abacn approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'master' into textio-header-removal

edc8a75

Abacn merged commit bf47289 into apache:master Nov 8, 2023
26 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add header removal for TextIO #29202

Add header removal for TextIO #29202

stankiewicz commented Oct 30, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023

stankiewicz commented Oct 31, 2023

ghost commented Oct 31, 2023 •

edited by ghost

Loading

stankiewicz commented Nov 3, 2023

Abacn Nov 3, 2023

ghost Nov 3, 2023

ghost Nov 3, 2023 •

edited by ghost

Loading

stankiewicz Nov 3, 2023

ghost Nov 3, 2023 •

edited by ghost

Loading

stankiewicz Nov 3, 2023

ghost Nov 3, 2023

stankiewicz commented Nov 3, 2023

stankiewicz commented Nov 3, 2023

stankiewicz commented Nov 6, 2023

ghost left a comment

Abacn left a comment

Abacn commented Nov 8, 2023

Add header removal for TextIO #29202

Add header removal for TextIO #29202

Conversation

stankiewicz commented Oct 30, 2023 • edited Loading

github-actions bot commented Oct 31, 2023

stankiewicz commented Oct 31, 2023

ghost commented Oct 31, 2023 • edited by ghost Loading

stankiewicz commented Nov 3, 2023

Abacn Nov 3, 2023

Choose a reason for hiding this comment

ghost Nov 3, 2023

Choose a reason for hiding this comment

ghost Nov 3, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

stankiewicz Nov 3, 2023

Choose a reason for hiding this comment

ghost Nov 3, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

stankiewicz Nov 3, 2023

Choose a reason for hiding this comment

ghost Nov 3, 2023

Choose a reason for hiding this comment

stankiewicz commented Nov 3, 2023

stankiewicz commented Nov 3, 2023

stankiewicz commented Nov 6, 2023

ghost left a comment

Choose a reason for hiding this comment

Abacn left a comment

Choose a reason for hiding this comment

Abacn commented Nov 8, 2023

stankiewicz commented Oct 30, 2023 •

edited

Loading

ghost commented Oct 31, 2023 •

edited by ghost

Loading

ghost Nov 3, 2023 •

edited by ghost

Loading

ghost Nov 3, 2023 •

edited by ghost

Loading