[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[chermenin]

Github user chermenin commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85812829
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

I understand. Is there possible this case as serialization with new 
serializer and deserialization with old one? Exception will be in this case, 
isn't it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[fhueske]

Github user fhueske commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85785788
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

Sure, we have to change `deserialize()` as well. 
If we can check for `2` (i.e, `null`) before we check for `1` (`SOME`), it 
should work, no?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[chermenin]

Github user chermenin commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85782914
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

If use `2` for `null` value we will get `true` from `readBoolean` method 
too.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[fhueske]

Github user fhueske commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85779839
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

If `writeBoolean` is correctly implemented (i.e., according to the JavaDocs 
in `java.io.DataOutput#writeBoolean(boolean)`) we should be good. However, you 
are right, that we may not change the `0` -> `false` and `1` -> `true` mapping.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[chermenin]

Github user chermenin commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85776021
  
--- Diff: 
flink-tests/src/test/scala/org/apache/flink/api/scala/runtime/CaseClassComparatorTest.scala
 ---
@@ -34,7 +34,7 @@ import org.mockito.Mockito
 
 class CaseClassComparatorTest {
 
-  case class CaseTestClass(a: Int, b: Int, c: Int, d: String)
+  case class CaseTestClass(a: Int, b: Int, c: Int, d: String, e: 
Option[Int])
--- End diff --

Ok, I will do it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[chermenin]

Github user chermenin commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85775790
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

Yes, but in this case we lose compatibility with previous implementations 
of serialization. Is it possible?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[fhueske]

Github user fhueske commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85770344
  
--- Diff: 
flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala
 ---
@@ -65,7 +65,7 @@ class OptionSerializer[A](val elemSerializer: 
TypeSerializer[A])
 case Some(a) =>
   target.writeBoolean(true)
   elemSerializer.serialize(a, target)
-case None =>
+case _ =>
--- End diff --

This would represent `null` the same way as `None`. However, `null` is not 
the same as `None` so this is incorrect. 

`writeBoolean` serializes a whole byte: `0` for `false` and `1` for `true`. 
We should change `writeBoolean` to `writeByte` and keep the current mapping 
and add `2` for `null`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[fhueske]

Github user fhueske commented on a diff in the pull request:

https://github.com/apache/flink/pull/2723#discussion_r85770866
  
--- Diff: 
flink-tests/src/test/scala/org/apache/flink/api/scala/runtime/CaseClassComparatorTest.scala
 ---
@@ -34,7 +34,7 @@ import org.mockito.Mockito
 
 class CaseClassComparatorTest {
 
-  case class CaseTestClass(a: Int, b: Int, c: Int, d: String)
+  case class CaseTestClass(a: Int, b: Int, c: Int, d: String, e: 
Option[Int])
--- End diff --

Please do not change this class but create a new `OptionSerializerTest` 
which extends `SerializerTestBase`. See for instance `IntSerializerTest` for 
how the test base is used.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2723: [FLINK-3617] Simple fix for OptionSerializer.

[chermenin]

GitHub user chermenin opened a pull request:

https://github.com/apache/flink/pull/2723

[FLINK-3617] Simple fix for OptionSerializer.

This fix prevent possible NullPointerException at serialization process, 
but after deserialization Option variable will be initialized with None value 
and not null (as was before serialization). This decision was made for 
compatibility between versions. What thoughts about it?

This PR solve 
[FLINK-3617](https://issues.apache.org/jira/browse/FLINK-3617).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/chermenin/flink flink-3617

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/2723.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2723


commit b742af340dbba4c927471d7f09e8d0b486dc6637
Author: Aleksandr Chermenin 
Date:   2016-10-28T13:52:26Z

[FLINK-3617] Simple fix for OptionSerializer.

This fix prevent possible NullPointerException at serialization process,
but after deserialization Option variable will be initialized with None
value and not null (as was before serialization).




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---