[CsvIO] Create CsvIOStringToCsvRecord Class #31857

lahariguduru · 2024-07-11T22:55:33Z

This PR closes #31854.

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.

github-actions · 2024-07-11T23:34:32Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @m-trieu for label java.
R: @ahmedabu98 for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

ahmedabu98

Thanks, left some comments

ahmedabu98 · 2024-07-12T21:20:18Z

sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecord.java

Should this class be exposed to the user (ie. public class) or is it internal only?

Since everything is currently internal, all classes are package private

ahmedabu98 · 2024-07-12T21:23:57Z

sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecord.java

Can we make it possible to create a CsvIOStringToCsvRecord transform without having to provide an errorHandlerTransform?

I agree, I added the transform to the CsvIOConfiguration class itself but will update this class to include it.

Unfortunately had to completely remove checking for errors within this class due to git rebase requirement -- the branch being unable to pull the most recent version of CsvIOParseConfiguration. The test for Bad record will also have to be in different class.

ahmedabu98 · 2024-07-12T21:26:27Z

sdks/java/io/csv/src/test/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecordTest.java

nit: cleanup

ahmedabu98 · 2024-07-12T22:02:27Z

sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecord.java

Should this be outputting CSVRecord?

Suggested change

extends PTransform<PCollection<String>, PCollection<Iterable<String>>> {

extends PTransform<PCollection<String>, PCollection<CSVRecord>> {

Output was originally CSVRecord, but due to CSVRecord being read-only and formatting differences in equals() method, it is not possible to compare CSVRecord(read-only) in test checks.

Hmmm I see. We should probably find a way around that. Looking at the CSVRecord class, there's some useful methods we'd be missing out on if we cast it to an Iterable<String>.

Instead, we can maybe try converting the output CSVRecord to an ArrayList before doing our test checks. For example:

PCollection<String> outputStrings = input .apply(underTest) .apply(MapElements.into(TypeDescriptors.lists(Typedescriptors.strings())).via( record -> ImmutableList.copyOf(record.iterator()))); // Passert checks with outputStrings

You can implement custom comparison in test to override the equals

I.e

assertThat(csvRecords) .comparingElementsUsing( Correspondence.from(...customComparator)

Thank you, I will implement both measures and see the best way to go about getting around the equals mismatch. @ahmedabu98 I agree, CSVRecord is also easier to convert into custom type so overall a better choice for corresponding classes that require CsvIOStringToCsvRecord.

Upon further documentation, in future versions the Serializable is deprecated in CSVRecord (versions later than 1.8, which is currently being used). This was the main reason for using Iterable rather than CSVRecord.

via CSVRecord 1.8: Note: Support for Serializable is scheduled to be removed in version 2.0. In version 1.8 the mapping between the column header and the column index was removed from the serialised state. The class maintains serialization compatibility with versions pre-1.8 for the record values; these must be accessed by index following deserialization. There will be loss of any functionally linked to the header mapping when transferring serialised forms pre-1.8 to 1.8 and vice versa.

Ahh I see, that is pretty unfortunate. I guess Iterable<String> makes sense to go with (even though the naming of this transform will be a little misleading)

Naming itself isn't the best but only data truly needed from the CsvRecord is the cell for future parsing.

sdks/java/io/csv/src/test/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecordTest.java

m-trieu · 2024-07-13T00:53:35Z

sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecord.java

Can this class be final?

This class can be final but it defies the convention of all other created classes that are currently only package private.

m-trieu · 2024-07-13T01:02:15Z

sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIOStringToCsvRecord.java

You can implement custom comparison in test to override the equals

I.e

assertThat(csvRecords) .comparingElementsUsing( Correspondence.from(...customComparator)

lahariguduru · 2024-07-15T21:18:22Z

Run Java_Examples_Dataflow PreCommit

ahmedabu98

LGTM

ahmedabu98 · 2024-07-15T21:50:42Z

Fixes #31854

* Create CsvIOStringToCsvRecord class * Create CsvIOStringToCsvRecord Class * Create CsvIOStringToCsvRecord Class * Create CsvIOStringToCsvRecord Class * Fixed BadRecord Output * Make class final --------- Co-authored-by: Lahari Guduru <lahariguduru@google.com>

github-actions bot added java io labels Jul 11, 2024

github-actions bot added the Next Action: Reviewers label Jul 11, 2024

lahariguduru changed the title ~~[CsvIO] Create CsIOStringToCsvRecord Class~~ [CsvIO] Create CsvIOStringToCsvRecord Class Jul 12, 2024

ahmedabu98 reviewed Jul 12, 2024

View reviewed changes

lahariguduru requested a review from ahmedabu98 July 12, 2024 23:58

m-trieu suggested changes Jul 13, 2024

View reviewed changes

lahariguduru added 5 commits July 15, 2024 16:20

Create CsvIOStringToCsvRecord class

57ff525

Create CsvIOStringToCsvRecord Class

91f41e2

Create CsvIOStringToCsvRecord Class

6687df7

Create CsvIOStringToCsvRecord Class

82d8157

Fixed BadRecord Output

ed3e6c5

lahariguduru force-pushed the string-to-csv-record branch from 29c0ba2 to ed3e6c5 Compare July 15, 2024 16:33

Make class final

63d2686

ahmedabu98 approved these changes Jul 15, 2024

View reviewed changes

ahmedabu98 merged commit ea76111 into apache:master Jul 15, 2024

	extends PTransform<PCollection<String>, PCollection<Iterable<String>>> {
	extends PTransform<PCollection<String>, PCollection<CSVRecord>> {

[CsvIO] Create CsvIOStringToCsvRecord Class #31857

[CsvIO] Create CsvIOStringToCsvRecord Class #31857

Uh oh!

Conversation

lahariguduru commented Jul 11, 2024

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented Jul 11, 2024

Uh oh!

ahmedabu98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahariguduru Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahariguduru Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahariguduru commented Jul 15, 2024

Uh oh!

ahmedabu98 left a comment

Choose a reason for hiding this comment

Uh oh!

ahmedabu98 commented Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lahariguduru Jul 12, 2024 •

edited

Loading

lahariguduru Jul 15, 2024 •

edited

Loading