-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-11457] Add option to skip key-value clone #13543
[BEAM-11457] Add option to skip key-value clone #13543
Conversation
@JozoVilcek Thanks, I'll take a look asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks reasonable and fine in general but after a discussion on BEAM-11457 we agreed to make this option independent for key and value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some additional comments, ptal.
Also, please update CHANGES.md
too.
...java/io/hadoop-format/src/main/java/org/apache/beam/sdk/io/hadoop/format/HadoopFormatIO.java
Outdated
Show resolved
Hide resolved
...java/io/hadoop-format/src/main/java/org/apache/beam/sdk/io/hadoop/format/HadoopFormatIO.java
Outdated
Show resolved
Hide resolved
* Determines if key-value clone should be skipped or not (default is 'false'). Hadoop formats | ||
* typically work with Writable data structures which are mutable. Therefore, this IO will clone | ||
* read key-values if they are not in the list of well known immutable types. However, in case | ||
* user does use key/value translation functions, resulting key-values might already be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be used only with withKeyTranslation()
or withValueTranslation()
, or it can be used independently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep it separate. I am not sure if there are some input formats out there which can be not reusing instances of does not use writables for serialization.
...java/io/hadoop-format/src/main/java/org/apache/beam/sdk/io/hadoop/format/HadoopFormatIO.java
Outdated
Show resolved
Hide resolved
ef0afe3
to
3cdccd5
Compare
retest this please |
1 similar comment
retest this please |
@aromanenko-dev change is ready for another review round. Apologies for delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.