[FLINK-24397] Remove TableSchema usages from Flink connectors #17459

fapaul · 2021-10-12T13:07:35Z

What is the purpose of the change

The TableSchema has been deprecated for a while now unfortunately many of our connectors still use it. This PR tries to address it and migrates the connectors to the new stack. (Schema/ResolvedSchema)

I'll create a follow-up ticket to do the same for the hive connector because it has some additional complexity. The only other connector which is missing is elasticsearch where we will address the TableSchema usage as part of https://issues.apache.org/jira/browse/FLINK-24323

Brief change log

Migrate TableSchema usages and remove outdated table connector code (Hbase, Kafka, Jdbc)

Verifying this change

All existing tests still pass

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2021-10-12T13:08:37Z

CI report:

0a7a93b Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot · 2021-10-12T13:12:08Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 69e676d (Tue Oct 12 13:12:07 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

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 commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

twalthr

Thanks for the PR @fapaul. I added some comments. In particular, I would suggest to scan the changes again whether ResolvedSchema is used within a TableFactory. This should not be necessary anymore.

...ase-base/src/main/java/org/apache/flink/connector/hbase/table/HBaseConnectorOptionsUtil.java

...nector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java

...connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableSink.java

...nnector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableSource.java

twalthr · 2021-10-12T13:21:47Z

...k-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcLookupFunction.java

- * <p>Support cache the result to avoid frequent accessing to remote databases. 1.The cacheMaxSize
- * is -1 means not use cache. 2.For real-time data, you need to set the TTL of cache.
- */
-public class JdbcLookupFunction extends TableFunction<Row> {


can we drop this? is there a similar RowData one?

Yes there is a RowData one https://github.com/apache/flink/blob/master/flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcRowDataLookupFunction.java

twalthr · 2021-10-12T13:24:31Z

...ors/flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/JdbcDataTypeTest.java

-                        "VARBINARY(10)",
-                        "The Derby dialect doesn't support type: VARBINARY(10)."),
+                        "BINARY",
+                        "Unsupported conversion from data type 'BINARY(1)' (conversion class: [B) to type information. Only data types that originated from type information fully support a reverse conversion."),


this exception seems wrong. we should not use a conversion from data type to legacy type information anymore

Unfortunately, I am not really sure what the correct behavior here is. The problem is that this complete test never used the new JDBC connector and always tested with the old one which is now removed. It appears that with the new connector the exception signatures have changed.

Are you proposing to drop the test class completely?

The exception indicates that are using a deprecated utility. We should analyze the stack trace where this exception is coming from.

...rc/main/java/org/apache/flink/streaming/connectors/kafka/table/KafkaDynamicTableFactory.java

fapaul · 2021-10-13T07:10:27Z

@twalthr thanks for your review, I addressed all comments, can you have another look?

…able connector

twalthr

Thanks for the update @fapaul. This leaves the code base a better place. I will merge this once the build is green.

twalthr · 2021-10-19T12:42:42Z

@flinkbot run azure

…le connector

twalthr

The build succeeded. Merging...

…moving legacy This closes apache#17459.

fapaul changed the title ~~Flink 24397 wh~~ [FLINK-24397] Remove TableSchema usages from Flink connectors Oct 12, 2021

twalthr reviewed Oct 12, 2021

View reviewed changes

rmetzger added component=Connectors/Common component=TableSQL/API labels Oct 12, 2021

fapaul force-pushed the FLINK-24397-wh branch from 69e676d to a307c35 Compare October 13, 2021 07:06

fapaul force-pushed the FLINK-24397-wh branch from a307c35 to 77238fc Compare October 18, 2021 12:41

Fabian Paul added 2 commits October 19, 2021 10:46

[FLINK-24397][connectors/kafka] Remove TableSchema usage from Kafka t…

517a23a

…able connector

[FLINK-24397][connectors/hbase] Remove TableSchema usage from Hbase t…

984aaa2

…able connector

fapaul force-pushed the FLINK-24397-wh branch from 77238fc to a10953a Compare October 19, 2021 08:48

twalthr approved these changes Oct 19, 2021

View reviewed changes

[FLINK-24397][connectors/jdbc] Remove TableSchema usage from Jdbc tab…

0a7a93b

…le connector

fapaul force-pushed the FLINK-24397-wh branch from a10953a to 0a7a93b Compare October 19, 2021 14:36

twalthr approved these changes Oct 20, 2021

View reviewed changes

twalthr closed this in 66d9188 Oct 20, 2021

FaxianZhao mentioned this pull request Dec 14, 2021

[FLINK-12309][JDBC] JDBCOoutputFormat and JDBCAppendTableSink float behavior is not align #8248

Closed

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-24397][connectors/jdbc] Cleanup the Jdbc table connector by re…

b861e7d

…moving legacy This closes apache#17459.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-24397] Remove TableSchema usages from Flink connectors #17459

[FLINK-24397] Remove TableSchema usages from Flink connectors #17459

fapaul commented Oct 12, 2021 •

edited

Loading

flinkbot commented Oct 12, 2021 •

edited

Loading

flinkbot commented Oct 12, 2021

twalthr left a comment

twalthr Oct 12, 2021

fapaul Oct 12, 2021

twalthr Oct 12, 2021

fapaul Oct 12, 2021

twalthr Oct 18, 2021

fapaul commented Oct 13, 2021

twalthr left a comment

twalthr commented Oct 19, 2021

twalthr left a comment

[FLINK-24397] Remove TableSchema usages from Flink connectors #17459

[FLINK-24397] Remove TableSchema usages from Flink connectors #17459

Conversation

fapaul commented Oct 12, 2021 • edited Loading

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Oct 12, 2021 • edited Loading

CI report:

flinkbot commented Oct 12, 2021

Automated Checks

Review Progress

twalthr left a comment

Choose a reason for hiding this comment

twalthr Oct 12, 2021

Choose a reason for hiding this comment

fapaul Oct 12, 2021

Choose a reason for hiding this comment

twalthr Oct 12, 2021

Choose a reason for hiding this comment

fapaul Oct 12, 2021

Choose a reason for hiding this comment

twalthr Oct 18, 2021

Choose a reason for hiding this comment

fapaul commented Oct 13, 2021

twalthr left a comment

Choose a reason for hiding this comment

twalthr commented Oct 19, 2021

twalthr left a comment

Choose a reason for hiding this comment

fapaul commented Oct 12, 2021 •

edited

Loading

flinkbot commented Oct 12, 2021 •

edited

Loading