[FLINK-11420][core] Fixed duplicate method of TraversableSerializer #7872

dawidwys · 2019-03-01T07:59:15Z

What is the purpose of the change

The duplicate method of TypeSerializer used an equality check rather
than reference check of the element serializer to decide if we need a
deep copy. This commit uses proper reference comparison.

Brief change log

79284d6 - Tuple0Serializer serialized null as just a proper Tuple0 instance, which if later deserialized resulted in a regular value rather than null. This commit changes it so that it is no longer possible to serialize null with Tuple0Serializer.
871890d - adds snapshot for serializer TestListCompositeSerializer that is used only in tests
8f970c6 - adds snapshot for serializer NothingSerializer which can be called transitively e.g. from EitherSerializer
f3549eb - some tests were comparing contents of the collections rather than the collections itself, what they claimed
1f122e7 - calls all methods from SerializerTestBase from within SerializerTestInstance
3b10292 - The duplicate method of TypeSerializer used an equality check rather
than reference check of the element serializer to decide if we need a
deep copy. This commit uses proper reference comparison.
5896e89 - This commit introduced a matcher that performs a deepEquals comparison similarly to Objects#deepEquals. The only difference is that it also performs deep equality check for flink's specific types.

Verifying this change

enabled additional test (including duplicate method test in SerializerTestInstance

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2019-03-01T07:59:34Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

❓ 1. The [description] looks good.
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @aljoscha [PMC]
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

aljoscha · 2019-03-01T08:07:24Z

flink-core/src/test/java/org/apache/flink/api/common/typeutils/SerializerTestInstance.java

-		testSerializedCopyIndividually();
-		testSerializedCopyAsSequence();
-		testSerializabilityAndEquals();
+		try {


Do you know why these were not called before? Also, why do we need the try-catch around this now?

I guess they were added later to the base class and the person that added those cases was probably not aware of existence of this class.

The try-catch is because a few of the new test cases throw exception, whereas the testAll doesn't. Did not want to change it in every place where the testAll is called. I think though in the long run we should revisit those places and actually get rid of this method at all.

so this isn't even compiling at the moment?

it is compiling

I just realized though there some test failures for different serializers, investigating...

I think it would probably be cleaner to not catch the exception and simply declare that it throws. Which might lead to other places where we need to declare, as you said.

I would suggest maybe replacing this with:

for (Method method : getClass().getMethods()) { if (method.getAnnotation(Test.class) == null) { continue; } try { method.invoke(this); } catch (IllegalAccessException e) { throw new RuntimeException("Unable to invoke test " + method.getName(), e); } catch (InvocationTargetException e) { sneakyThrow(e.getCause()); } }

where sneakyThrow is defined as follows

private static <E extends Throwable> void sneakyThrow(Throwable e) throws E { throw (E) e; }

dawidwys · 2019-03-01T16:09:28Z

Could you have a look at this PR @igalshilman @aljoscha

igalshilman

Thanks for improving the test coverage!
The changes look good to me, I've left some inline comments for your consideration.

igalshilman · 2019-03-01T18:23:34Z

flink-core/src/test/java/org/apache/flink/api/common/typeutils/SerializerTestInstance.java

-		testSerializedCopyIndividually();
-		testSerializedCopyAsSequence();
-		testSerializabilityAndEquals();
+		try {


I would suggest maybe replacing this with:

for (Method method : getClass().getMethods()) { if (method.getAnnotation(Test.class) == null) { continue; } try { method.invoke(this); } catch (IllegalAccessException e) { throw new RuntimeException("Unable to invoke test " + method.getName(), e); } catch (InvocationTargetException e) { sneakyThrow(e.getCause()); } }

where sneakyThrow is defined as follows

private static <E extends Throwable> void sneakyThrow(Throwable e) throws E { throw (E) e; }

igalshilman · 2019-03-01T18:27:23Z

flink-core/src/test/java/org/apache/flink/api/common/typeutils/SerializerTestInstance.java

-		} catch (Exception ex) {
-			throw new RuntimeException(ex);
-		}
+	public void testAll() throws Exception {


I don't think this is needed, check my previous comment regarding sneakyThrow

igalshilman · 2019-03-01T18:37:47Z

flink-core/src/test/java/org/apache/flink/testutils/TupleEqualityMatcher.java

+	private static boolean deepEquals0(Object e1, Object e2) {
+		assert e1 != null;
+		boolean eq;
+		if (e1 instanceof Object[] && e2 instanceof Object[]) {


I think this can be replaced with e1.getClass().isArray()
and the recursively call deepEquals element wise (to catch Tuples of Tuples)
or if this is not needed for the test, then maybe just replace with Arrays.deepEquals

igalshilman · 2019-03-01T18:46:45Z

flink-core/src/test/java/org/apache/flink/testutils/TupleEqualityMatcher.java

+		description.appendValue(wanted);
+	}
+
+	private static boolean deepEquals(Object o1, Object o2) {


~~I think that this logic can be simplified with something like~~

private static Stream<Object> flatten(Object o) { if (o == null) { return Stream.empty(); } if (o.getClass().isArray()) { return Arrays.stream((Object[]) o) .flatMap(e -> flatten(e)); } if (o instanceof Tuple) { Tuple t = (Tuple) o; return IntStream.range(0, t.getArity()) .mapToObj(t::getField) .flatMap(e -> flatten(e)); } return Stream.of(o); }

~~and then~~

private static boolean deepEquals(Object left ,Object right) { Iterator<Object> l = flatten(left).iterator(); Iterator<Object> r = flatten(right).iterator(); ... }

update: I gave it a bit more thought, and this isn't exactly what you need.

I haven't tested that tho 😅

Tuple0Serializer serialized null as just a proper Tuple0 instance, which if later deserialized resulted in a regular value rather than null. This commit changes it so that it is no longer possible to serialize null with Tuple0Serializer.

…yoGenericTypeSerializerTest

…rTestInstance by reflection

The duplicate method of TypeSerializer used an equality check rather than reference check of the element serializer to decide if we need a deep copy. This commit uses proper reference comparison.

…ng Tuples into account This commit introduced a matcher that performs a deepEquals comparison similarly to Objects#deepEquals. The only difference is that it also performs deep equality check for some flink specific classes such as e.g. Tuples, Rows.

aljoscha · 2019-03-04T15:39:53Z

@flinkbot approve consensus

rmetzger added the review=description? label Mar 1, 2019

aljoscha reviewed Mar 1, 2019

View reviewed changes

aljoscha self-requested a review March 1, 2019 09:23

dawidwys force-pushed the flink11420 branch 2 times, most recently from dcfb614 to 8ca7ced Compare March 1, 2019 16:05

igalshilman approved these changes Mar 1, 2019

View reviewed changes

dawidwys force-pushed the flink11420 branch 4 times, most recently from f771de4 to 5593e8b Compare March 4, 2019 13:53

dawidwys added 7 commits March 4, 2019 15:30

[hotfix][core] Added snapshot for TestListCompositeSerializer

871890d

[hotfix][core] Added snapshot for NothingSerializerSnapshot

8f970c6

[hotfix][core] Comparing whole collections rather than contents in Kr…

f3549eb

…yoGenericTypeSerializerTest

[hotfix][tests] Call all methods from SerializerTestBase in Serialize…

1f122e7

…rTestInstance by reflection

[FLINK-11420][core] Fixed duplicate method of TraversableSerializer

3b10292

The duplicate method of TypeSerializer used an equality check rather than reference check of the element serializer to decide if we need a deep copy. This commit uses proper reference comparison.

dawidwys force-pushed the flink11420 branch from 5593e8b to 5896e89 Compare March 4, 2019 14:47

dawidwys mentioned this pull request Mar 4, 2019

[FLINK-11420][core][bp-1.8] Fixed duplicate method of TraversableSerializer #7894

Merged

dawidwys merged commit 39e1384 into apache:master Mar 4, 2019

rmetzger added the component=API/TypeSerializationSystem label Mar 18, 2019

[FLINK-11420][core] Fixed duplicate method of TraversableSerializer #7872

[FLINK-11420][core] Fixed duplicate method of TraversableSerializer #7872

Uh oh!

Conversation

dawidwys commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Progress

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!

dawidwys Mar 1, 2019 • 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!

dawidwys commented Mar 1, 2019

Uh oh!

igalshilman 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!

igalshilman Mar 1, 2019 • 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!

aljoscha commented Mar 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dawidwys commented Mar 1, 2019 •

edited

Loading

flinkbot commented Mar 1, 2019 •

edited

Loading

dawidwys Mar 1, 2019 •

edited

Loading

igalshilman Mar 1, 2019 •

edited

Loading