flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From fhueske <...@git.apache.org>
Subject [GitHub] flink issue #2810: [FLINK-3848] Add ProjectableTableSource interface and tra...
Date Tue, 06 Dec 2016 15:32:28 GMT
Github user fhueske commented on the issue:

    Hi @tonycox, @beyond1920, and @KurtYoung,
    I think I found a good way to split the issue. Let me first summarize what both of your
PRs implement:
    @tonycox (PR #2810)
    - good test coverage with tests that check the optimized plan (`TableTestBase`)
    - adapted `RowCsvInputFormat` + `CsvTableSource` to support projection
    - first changes for projection pushdown in streaming
    - good cost function
    @beyond1920 (PR #2923)
    - Extracts fields from `RexProgram` and converts `RexProgram` (seems to reused in PR #2810)
    - Rules work on `DataSetRel` instead of `LogicalRel` (#2810 rules seem to be inspired
by this rule)
    - too many ITCases
    - no changes for streaming
    - no projection support for `CsvTableSource`.
    From my point of view PR #2923 provides a solid basis for this feature which seem to be
partially reused in PR #2810 but lacks a few things that have been implemented in PR #2810.

    Therefore, I would propose the following:
    PR #2923 is modified to cover the following:
    - `ProjectableTableSource`
    - `RexProgramProjectExtractor` + missing unit test
    - Rule to push projection into `BatchTableSourceScan`
    - Remove all ITCases except for one (the other PR will add more tests)
    - Adapt the cost model for the `BatchTableSourceScan` as in PR #2810.
    PR #2810 builds on PR#2923 and adds the following:
    - Translation for `StreamTableSourceScan` + one ITCase
    - `CsvTableSource` + `RowCsvInputFormat` changes (including tests and tests refactoring)
    - The extensive plan tests based on `TableTestBase` for batch and streaming
    If we agree on this plan, it would be good if PR #2923 would be adapted as soon as possible
such that PR #2810 can be built on top.
    What do you think @tonycox, @beyond1920, @KurtYoung?
    Best, 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.

View raw message