[GitHub] flink issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/4355 Thanks for the update @kaibozhou. I think this PR is good to merge. I'll do some final small changes and will merge this. Thanks, Fabian --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 All comments addressed. Do you have time to merge it @wuchong @fhueske ? Thanks. --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/4355 Hi @wuchong, I think we don't need to call `open()` and `close()` in `AggregateAggFunction`. `GeneratedAggregations` is an internal class which is not exposed to users. It would be a bug in the translation logic if a `GeneratedAggregations` which requires `open()` or `close()` would be passed to a `AggregateAggFunction`. A user couldn't do anything to prevent this. +1 for refactoring `AggregateCodeGenerator`. --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 Thanks @fhueske for the suggestion. All comments addressed. --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user wuchong commented on the issue: https://github.com/apache/flink/pull/4355 The new created method `open(ctx)` and `close()` of `GeneratedAggregations` are not called by `AggregateAggFunction` which is used in window aggregate. I suggest to call the `open(ctx)` method but pass a `RuntimeContext` which throw exceptions in every method to tell users `User Defined AggregateFunction is not supported to call open() and close() in window`. But this can be addressed in another issue. BTW, I think the `AggregateCodeGenerator#generateAggregations` is too long with 500+ LOC. I would like to refactor it if you have no objection @kaibozhou @fhueske . I have created [FLINK-7509](https://issues.apache.org/jira/browse/FLINK-7509) . --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 @fhueske @wuchong Thank you for your suggestion, I have update the PR and add test case, thanks. Do you have time to look at this? Thanks, Kaibo --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 In MapViewSerializer.scala, the function "override def serialize(record: MapView[K, V], target: DataOutputView):" need to access var map which defined in MapView.scala. Becase mapSerializer.serialize(record.map, target) only accept java.util.Map, otherwise, we need to new java.util.Map to call this method, this is not very efficient. The problem is âprivate[flink] var mapâ will be seen by Java users, In fact, we do not want users to see the implementation of MapView internal. Maybe use "protected" keyword in Java can solve the problem. --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 1. Use HeapMapView/HeapListView as default implementation 2. add initialize and cleanUp interface to GeneratedAggregations --- 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 issue #4355: [FLINK-7206] [table] Implementation of DataView to suppor...
Github user kaibozhou commented on the issue: https://github.com/apache/flink/pull/4355 Comments addressed. Thanks. --- 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. ---
