You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by Shiti <gi...@git.apache.org> on 2015/06/25 13:46:19 UTC
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
GitHub user Shiti opened a pull request:
https://github.com/apache/flink/pull/867
[FLINK-2230] handling null values for TupleSerializer
When serializing, we add a `byte[] (BitSet.toByteArray)` before the fields which indicates `null` fields.
When deserializing, we fetch the `byte[]` and obtain the underlying `BitSet (BitSet.valueOf)`. `BitSet.get(index)` is `false` when the value is `null`. For each field element we check if the its marked as `null` in the `BitSet` and then pass it to the fieldSerializer.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Shiti/flink FLINK-2230
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/867.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 #867
----
commit a4d2731eb75032f958e96323a075eb8bc7d11c73
Author: Shiti <ss...@gmail.com>
Date: 2015-06-25T10:36:10Z
[FLINK-2230]handling null values for TupleSerializer
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/867
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-139547674
I agree with @StephanEwen that the TupleSerializer should not support null values. However, it might be good to offer a null-aware alternative.
Can we close this PR for now until we agreed on a solution for this issue?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-115248743
@StephanEwen hinted that the best way to go would be to decuple the RowTypeInfo completely from the TupleTypeInfo/TupleSerializerBase. This way, we get null-value support in the Table API without changing the existing code for tuples. This would require changing the RowTypeInfo to no longer be a child of CaseClassTypeInfo and creating non-subclass RowSerializer and RowComparator.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-116520207
@Shiti No need to apologize, that is all right. We are discussing a tricky thing here...
It is true, right now, one needs to be aware of missing values in the application logic. I personally like that, an `Option` makes it very explicit and easy to grasp. Also the byte overhead for the option field is okay, as most fields will not be nullable and not carry this overhead. The serialization logic is also more CPU efficient that way.
What is true is that there is a problem when converting from a `Table` to a `DataSet[Tuple]`. Tables with nullable fields could only be converted to data sets of class types, which would support nullable fields.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-115227753
We actually had a discussion about this quite a few times. I also raised my concerns in the discussion of the issue, to which no one reacted.
The serialization subsystem (and tuples) are of the most critical nature in Flink. There are so many side effects and considerations. Comparators that interact with serializers, normalized keys, subclasses and tagging, object creation (GC impact). None of that is taken into account here.
For something as crucial as this, we cannot make changes without being discussed thoroughly before, and at best, also documented. It makes sense to do this before the code writing.
Sorry if I appear like the bad guy here. But we are at the verge of getting into spaghetti code and inconsistencies in one of the most crucial parts, and we cannot do that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-140018844
Yes, I'm afraid it's better to close it and provide an alternative for the Table API, for example.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by Shiti <gi...@git.apache.org>.
Github user Shiti commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-115936916
@StephanEwen, Apologies, I didn't notice the earlier message in jira. Something wrong with my GMail settings, most of the messages from jira and mailing list went into Spam.
Kindly excuse my limited understanding of this framework and the intention/drivers behind the decisions made.
Going through the mailing list and the ticket I realized that though there may be some valid cases of missing data types, it will not be desirable to change the `TupleTypeInfo` and the whole Tuple/Case Class Serialization code-base to support null and we should identify an alternative approach to handle this.
From my limited understanding, the recommended way of working with missing values is to use `(Option[Int], Option[Int]])` instead of `(Int, Int)`, when we know there can be missing values in the data. Is that correct?
If that is correct, I have a few doubts,
1. Doesn't this push the handling of missing data to the application code (which may be good or bad), but makes the application code more verbose?
2. Wouldn't the size of Option[Int] in memory (and also in serialization) be more than just Int?
3. If Flink does not support null values except for in the Table API, won’t there be inconsistency when users try to convert a `Table` to a `DataSet[Tuple]`?
One alternative approach I can think of is introducing another TypeInfo which supports null values (say TupleTypeInfoWithNull) so users can choose to use that when they know/think that the data may contain 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-115228662
Also, apparently no tests were ever run after these changes. All fail on the build server on basic checkstyle rules even.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2230] handling null values for TupleSer...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/867#issuecomment-140223417
OK, I will close this PR tomorrow, unless somebody speaks up.
@Shiti, I am sorry for the time you invested in this PR, but we cannot add it to Flink's codebase without major implications.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---