[FLINK-17361] [jdbc] Added custom query on JDBC tables #11986

fpompermaier · 2020-05-04T16:38:30Z

What is the purpose of the change

Enable users to create a JDBC source table using a custom query / prepared statement.

Brief change log

Managed 'connector.read.query' to create a (read-only) source table using a custom query.
Currently a read-only Table cannot be created. When a final decision will be taken on that we should set such a flag for tables having such a property
When a final decision will be made for renaming of connector properties this property could probably become simply 'scan.query', but I decided to left the renaming of those properties to another specific PR

Verifying this change

This change is already covered by existing tests, such as JDBCTableSourceSinkFactoryTest and JDBCTableSourceITCase.

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

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

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? docs

flinkbot · 2020-05-04T16:41:58Z

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 8c15ff1 (Fri Oct 16 10:53:51 UTC 2020)

Warnings:

Documentation files were touched, but no .zh.md files: Update Chinese documentation or file Jira ticket.

_{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

flinkbot · 2020-05-04T16:51:56Z

CI report:

ef38bb8 Azure: FAILURE
8c15ff1 Azure: PENDING

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

aljoscha

Thanks for your contribution! I like that you didn't forget about also adding documentation. 😃

I had one comment in the test that is a bit more important and a minor nitpick about wording in the documentation.

aljoscha · 2020-05-07T09:21:50Z

...ectors/flink-jdbc/src/test/java/org/apache/flink/api/java/io/jdbc/JDBCTableSourceITCase.java

+				")"
+		);
+
+		StreamITCase.clear();


I think using the static StreamITCase functionality here is an anti-pattern that we should avoid. (I know that it's widely used in the code base, but we should rid of it at some point.)

Ideally, with the FLIP-84 work it should be as easy as

TableResult tableResult = tEnv.executeSql("SELECT timestamp6_col, decimal_col FROM " + INPUT_TABLE); tableResult.collect();

but that is not yet in master. I'm inclined to wait for a but but basically have this PR ready in a mergable state.

I just copied the code from another test and I modifier it. I didn't ask myself too much why there's the need to call StreamITCase.clear()

I understand, the code code that is there is already not good. 😞

Maybe we can merge this and open an issue to fix the tests in this class to the right way when FLIP-84 will be ready

docs/dev/table/connect.md

…ad of StreamITCase Using the static sink approach of StreamITCase is potentially problematic with concurrency, plus the code is just plain nicer like this.

aljoscha · 2020-05-19T15:44:12Z

@fpompermaier I rebased this on master and modified the ITCase to use the new TableResult. If you're ok with the changes I'll wait for azure and then merge.

aljoscha · 2020-05-19T17:50:46Z

Thanks a lot! I merged it before you answered, I hope that's ok. (I wanted to get it in in time for Flink 1.11)

fpompermaier · 2020-05-19T19:31:14Z

Thanks a lot @aljoscha

twalthr · 2020-05-20T06:39:22Z

...onnector-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcTableSourceITCase.java

+		assertThat(
+				results,
+				containsInAnyOrder(
+						"2020-01-01T15:35:00.123456,100.1234",


@aljoscha for the future: please use instances instead of strings

Noted, that's a very good point.

rmetzger added the review=description? label May 4, 2020

rmetzger added the component=TableSQL/API label May 4, 2020

aljoscha reviewed May 7, 2020

View reviewed changes

fpompermaier mentioned this pull request May 8, 2020

[FLINK-17537][connectors / jdbc] Refactor flink-jdbc connector structure #12036

Merged

fpompermaier and others added 2 commits May 19, 2020 17:40

[FLINK-17361] Add custom query on JDBC tables

7bed30e

[FLINK-17361] Refactor JdbcTableSourceITCase to use TableResult inste…

8c15ff1

…ad of StreamITCase Using the static sink approach of StreamITCase is potentially problematic with concurrency, plus the code is just plain nicer like this.

aljoscha force-pushed the flink-17361 branch from ef38bb8 to 8c15ff1 Compare May 19, 2020 15:43

aljoscha merged commit 8c15ff1 into apache:master May 19, 2020

twalthr reviewed May 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-17361] [jdbc] Added custom query on JDBC tables #11986

[FLINK-17361] [jdbc] Added custom query on JDBC tables #11986

fpompermaier commented May 4, 2020

flinkbot commented May 4, 2020 •

edited

flinkbot commented May 4, 2020 •

edited

aljoscha left a comment

aljoscha May 7, 2020

fpompermaier May 7, 2020

aljoscha May 7, 2020

fpompermaier May 7, 2020

aljoscha commented May 19, 2020

aljoscha commented May 19, 2020

fpompermaier commented May 19, 2020

twalthr May 20, 2020

aljoscha May 22, 2020

[FLINK-17361] [jdbc] Added custom query on JDBC tables #11986

[FLINK-17361] [jdbc] Added custom query on JDBC tables #11986

Conversation

fpompermaier commented May 4, 2020

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 May 4, 2020 • edited

Automated Checks

Review Progress

flinkbot commented May 4, 2020 • edited

CI report:

aljoscha left a comment

Choose a reason for hiding this comment

aljoscha May 7, 2020

Choose a reason for hiding this comment

fpompermaier May 7, 2020

Choose a reason for hiding this comment

aljoscha May 7, 2020

Choose a reason for hiding this comment

fpompermaier May 7, 2020

Choose a reason for hiding this comment

aljoscha commented May 19, 2020

aljoscha commented May 19, 2020

fpompermaier commented May 19, 2020

twalthr May 20, 2020

Choose a reason for hiding this comment

aljoscha May 22, 2020

Choose a reason for hiding this comment

flinkbot commented May 4, 2020 •

edited

flinkbot commented May 4, 2020 •

edited