[GitHub] flink issue #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3520 yes --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3520 Sure, so the result is there will be 2 commits to master repository? --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3520 Can you squash the commits of the PR into two commits before merging? One for @tonycox and one for your changes? 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3520 Hi @fhueske, thanks for the review. I addressed all your comments and will rebase to master to let travis check. Will merge this after build success. --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3520 OK, I thought about this. Let's stick to your mutable list approach. As you said it is safe, i.e., in worst case the table source does not remove expressions and we'll have a bit of overhead due to the unnecessary filters. --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3520 Sure, a `FilterableTableSource` can implement its accepted predicates as it likes. However, I think we could expect that `getPredicates()` returns the accepted filter expressions as it got them. This makes it easier to check a correct behavior of the `FilterableTableSource`. Internally, the accepted expressions can be decomposed or otherwise changed. Similarly, we would need to check that the Expressions which remain in the mutable list are also present in the input list. --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3520 Hi @fhueske, you just pointed out a question i had when tonycox implementing the first version. Why we are preventing `FilterableTableSource` from modifying the expressions? I think it's totally up to them whether changing the expressions they took or keep them just they were. We have not reason to restrict the `FilterableTableSource`'s behavior if they just do the things right. Pass in a java list and told user who extending this to pick and remove the expressions the support is not super nice. But even if user just pick expressions but not remove them, we still get the correct answer. What do you think? --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3520 Thanks for the update @KurtYoung! I think you have a good point about not extending `TableSourceScan`. The filter pushdown flag might not be the only one. Let's keep this information in `FilterableTableScan`. I'm not so sure about the interface of `applyPredicate(predicates: JList[Expression])`. I think we should not ask users to remove the accepted Expressions from the `predicates` list. How about we add a method `getPredicates(): JList[Expression]` which returns the accepted expressions. This method could also replace `isFilterPushedDown` if we expect `null` if `getPredicates()` was not called and an empty list if it was called but no predicate was accepted. IMO, `getPredicates()` has also the advantage that we can check that the `FilterableTableSource` did not modify the accepted predicates. What do you think? --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3520 Hi @fhueske @godfreyhe, thanks for the review, i addressed most all your comments. @fhueske Except for letting `TableSourceScan` be aware of whether filter has been pushed down. I'm not sure to let the `TableSourceScan` has this kind of information, i'd prefer to let them stay within the all kinds of `TableSource`. One drawback to let `TableSourceScan` has such kind of information is when we do the `TableSourceScan` copy, we need to take care all these information, make sure they also be copied correctly. In the future, if we add more extension to `TableSource` like we can push part of query down, we will face this problem. Regarding to the interface of `FilterableTableSource`, i agree with you that the trait containing some logic is not friendly with java extensions. So i removed the default implementation of `isFilterPushedDown`, the inherited class should take care of this method. And regarding the `Tuple2` thing, how about we pass in a mutable java list, and let table source to *pick out* expression from it and return a copy of table source which contains these pushed down predicates. --- 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 #3520: [FLINK-3849] [table] Add FilterableTableSource interface ...
Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3520 To address the problem of not reusing `TableSource` when we create a new scan for table source, i changed some inheritance for current `BatchScan`, `StreamScan`, `BatchTableSourceScan`, `StreamTableSourceScan` and so on. The new structure is moe likely as the relationship between `FlinkTable` and `TableSourceTable`, `DataSetTable`, `DataStreamTable`. After changing the structure, it also make it possible to unify the push project into scan rule for both batch and stream mode. --- 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. ---
