Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
The `WeakHashMap` with the `WeakReferences` seem to be working correctly.
I've checked that the generated classes are reused from the cache inside a
single job, but are not reused across jobs.
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
> Can we simplify it to the code below?
> So, we don't need to introduce cacheHit variable.
Thanks @heytitle , done in ff3a35e93e858e30debcb1e85cc056045c1fe2a0 .
---
Github user KurtYoung commented on the issue:
https://github.com/apache/flink/pull/3511
> So I think a simpler and better approach is to just make sure that most
types have a good implementation of putNormalizedKey, and then
NormalizedKeySorter.compareRecords would be called only rare
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
> IMHO, in addition to these changes, there are still some potential
improvements we can do about the sorter, like deserialization when comparing
the real records.
Do you mean `NormalizedKeyS
Github user KurtYoung commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle @ggevay Great work!
We are working on a project to fully code generate the algorithm Flink
runtime used, and borrowed lots of idea of this PR, thanks! IMHO, in addition
to these chan
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
Thanks @fhueske and @heytitle!
Most of the comments have been addressed. The two outstanding issues are
1. Addressing
https://github.com/apache/flink/pull/3511#discussion_r139933797
2.
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
Oh, you are right @heytitle, thanks! Fixed in
9016cce503b4d471b5a49f0abccc196945ada97e
These `WeakReferences` are scary :)
---
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
@ggevay Thanks for the commits. They look good. I also added you to my
repository.
For `FixedLengthRecordSorter`, I agree with you that we should implement
that when #2617 is merged.
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
I pushed some more commits to
[https://github.com/ggevay/flink/tree/sorter-codegen](https://github.com/ggevay/flink/tree/sorter-codegen).
@heytitle , could you please push them here? (Or you could add
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
Actually, now I'm thinking we should defer implementing code generation for
`FixedLengthRecordSorter` to after https://github.com/apache/flink/pull/2617 is
merged. What do you think @greghogan ?
---
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
I'm not exactly sure how to do `FixedLengthRecordSorter`.
@greghogan , could you please clarify what you meant in the following?
> Rather than extending NormalizedKeySorter, we could pull t
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @ggevay,
Thanks for the commits. Do you have any plan for `FixedLengthRecordSorter`
implementation?
I'm not sure how much work need to be done there.
---
If your project is set u
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
I pushed some commits here:
[https://github.com/ggevay/flink/tree/sorter-codegen](https://github.com/ggevay/flink/tree/sorter-codegen)
I fixed one issue by adding the `CLUSTER_WITH_CODEGEN
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
Thank @heytitle , I'll try to do the code generation for
`FixedLengthRecordSorter` tomorrow, and I will also take a look at the failing
tests.
Btw. you can force push (`git push -f`) the reba
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @ggevay,
I have rebased master and resolved all coding style related issues and
pushed the code to
https://github.com/heytitle/flink/tree/FLINK-5734-rebase-master-temp
For the n
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @ggevay,
I'm currently fixing coding style. Maybe you can help me take a look at
what Greg suggested related `FixedLengthRecordSorter`
---
If your project is set up for it, you can rep
Github user ggevay commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @heytitle,
What is the status here? I will be on vacation next week, so I would have
some time to work on this. Should I work on finishing this, or are you also
working on it? I would like
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @greghogan,
Thank very much for the feedback.
> Are you able to continue working on this feature?
Yes, I would like to complete the feature and will take a look into th
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle apologies for the long delay. I've been working to improve the
Gelly examples to process with each of the standard data types (from byte to
string). I think we can validate both the corre
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle yes, let's try that first.
---
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
enabl
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
Hi @greghogan,
Thanks for the explanation. I like the idea. I also think we might not need
`NormalizedKeySorterBase`, we can just extend generated sorters from
`NormalizedKeySorter` and ove
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle, the code generation is only for a few methods, right? So the
other methods in the sorter template could be moved into a
`NormalizedKeySorterBase` which would be subclasses by the templat
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
I also think about the abstract class but I'm not sure how to do it
properly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If you
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle, you can `rebase -i dd20^` and then delete the first two lines
which removes those commits from the history.
An initial thought from skimming the code: should we create an abstrac
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
@greghogan May I ask you how to remove `FLINK-3722` commits?. Only way I
can think of is `git rebase -i`, but this will rewrite history of this PR.
---
If your project is set up for it, you can rep
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
@heytitle please remove the old FLINK-3722 commits and rebase to master.
---
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 proj
Github user heytitle commented on the issue:
https://github.com/apache/flink/pull/3511
I haven't posted it yet. I will do it next week.
---
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 fe
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/3511
Has the FLIP been
[posted](https://cwiki.apache.org/confluence/display/FLINK/Flink+Improvement+Proposals)
and officially discussed on the mailing list?
---
If your project is set up for it, you c
28 matches
Mail list logo
