You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by greghogan <gi...@git.apache.org> on 2016/05/11 17:39:51 UTC
[GitHub] flink pull request: [FLINK-3868] [core] Specialized CopyableValue ...
GitHub user greghogan opened a pull request:
https://github.com/apache/flink/pull/1983
[FLINK-3868] [core] Specialized CopyableValue serializers and comparators
Update ValueTypeInfo to use specialized serializers and comparators, many of which were already present.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/greghogan/flink 3868_specialized_copyablevalue_serializers_and_comparators
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1983.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 #1983
----
commit c3359933b67f4488e1ed7cd4f2632ee21cdb548e
Author: Greg Hogan <co...@greghogan.com>
Date: 2016-05-04T20:56:16Z
[FLINK-3868] [core] Specialized CopyableValue serializers and comparators
Update ValueTypeInfo to use specialized serializers and comparators,
many of which were already present.
----
---
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 #1983: [FLINK-3868] [core] Specialized CopyableValue seri...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1983
---
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 issue #1983: [FLINK-3868] [core] Specialized CopyableValue serializers...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/1983
I think the code looks good.
There are no tests so far, though. I am wondering if some would help to catch copy/paste errors.
It is very easy to write comparator tests simply extending the `ComparatorTestBase`.
For the future: I am wondering if we can "generate" these serializers and comparators somehow (and their test classes), like we do for the tuple classes. Writing them seems like very repetitive work.
---
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-3868] [core] Specialized CopyableValue ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1983#issuecomment-221873359
Thanks, Greg, this looks good all in all!
A few thinks we should do before merging this, in my opinion:
- I think all classes should be annotated with `@Internal`, because they should not be used directly by users.
- The `NullValueComparator` can be simplified to not really operate on the values at all. All NullValues are always the same. They are all equal, hash to a constant value, etc.
- We were trying to get rid of the `Record` type - it was part of a very old legacy API. It is still in there because some people ended up using it, but I would like to add as little dependency to it as possible. Hence I'd suggest to drop the `RecordSerializer`.
---
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 issue #1983: [FLINK-3868] [core] Specialized CopyableValue serializers...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/1983
Will verify and merge ...
---
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 issue #1983: [FLINK-3868] [core] Specialized CopyableValue serializers...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/1983
@StephanEwen updated per your suggestions.
---
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.
---