New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-18999][table-planner-blink][hive] Temporary generic table does… #13216
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 1df875a (Fri Aug 21 12:22:33 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
This PR changed behavior: Planner use Java SPI to load temporary source/sink instead of Catalog.getTableFactory. |
Personally I am not convinced this is a good behaviour. I think temporary tables should use the factory of the catalog they were registered in. Imo if hive cannot deal with such tables, it should fall back in the factory to the SPI approach. As far as I am concerned it is a Hive shortcoming. |
31eef5a
to
167e78a
Compare
@JingsongLi @dawidwys Could you please help review this PR? |
167e78a
to
167af46
Compare
Can you create separate PR for |
167af46
to
93e3ffb
Compare
@JingsongLi I have separated the changes to planner from those to the hive connector. |
f07cf90
to
555452d
Compare
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/FactoryUtil.java
Outdated
Show resolved
Hide resolved
555452d
to
528277a
Compare
* | ||
* <p>It considers {@link Catalog#getFactory()} if provided. | ||
*/ | ||
public static DynamicTableSink createTableSink( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only source, sink too.
I can see the bug in HiveDynamicTableFactory
because not remove old method.
528277a
to
8d6ca96
Compare
8d6ca96
to
6656a1f
Compare
@JingsongLi I have updated the sink method too. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lirui-apache for the contribution, looks good to me.
…n't work with HiveCatalog
What is the purpose of the change
Fix the issue that generic temporary table can't be used with hive catalog
Brief change log
Verifying this change
Existing and added test cases
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation