Fixes GitHub issue #30257 #32074
Fixes GitHub issue #30257 #32074
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32074 +/- ##
============================================
+ Coverage 57.18% 58.66% +1.48%
- Complexity 1474 3023 +1549
============================================
Files 963 1121 +158
Lines 152697 172724 +20027
Branches 1076 3275 +2199
============================================
+ Hits 87313 101336 +14023
- Misses 63202 68084 +4882
- Partials 2182 3304 +1122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
R: @je-ik |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
Hi @je-ik , it took a while for me to figure out that there were some check style errors in the code😅. I've fixed those but looks like there are some checks failing that passed in the previous commit. Can you please guide me here if it's anything that I should fix on my end? Thanks! |
|
Hi @kirupha2000 , I'm sorry, I'm OOO, currently. I will take a look at this next week. Thanks for the contribution! |
|
Also please squash the change to single commit. There is a documentation about how to structure granularity of commits in https://github.com/apache/beam/blob/master/contributor-docs/committer-guide.md. Thanks! |
kirupha2000
force-pushed
the
master
branch
2 times, most recently
from
236573c
to
6be95b9
Compare
| SerializableFunction<String, Integer> fn = Integer::parseInt; | ||
|
|
||
| SerializableComparator<String> cmp = SerializableComparator.comparing(fn); | ||
| Assert.assertTrue(cmp instanceof Serializable); |
There was a problem hiding this comment.
This does not imply that the instance can be serialized. It would be better to use the SerializableUtils to check the serialization and deserialization.
There was a problem hiding this comment.
You could have left the original tests as well, those were fine.
There was a problem hiding this comment.
Would it be sufficient to check if the returned Comparator is serializable using org.apache.beam.sdk.util.SerializableUtils#ensureSerializable(T) or should we check using something else?
There was a problem hiding this comment.
Yes, that is exactly the method to use. 👍
There was a problem hiding this comment.
Re-added the original tests and used org.apache.beam.sdk.util.SerializableUtils#ensureSerializable(T).
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/SerializableComparator.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/SerializableComparator.java
Show resolved
Hide resolved
Adds a static comparing method to the SerializableComparator interface
Fixes #30257
Adds a static
comparingmethod to theSerializableComparatorinterface. This almost similar to the inbuilt Java'sComparator#comparing(Function)method except that it usesSerializableFunction.