[FLINK-17103][table-planner-blink] Supports dynamic options for Blink… #11711

danny0405 · 2020-04-13T05:16:11Z

… planner

What is the purpose of the change

Supports syntax "table /*+ OPTIONS('k1'='v1', 'k2'='v2') */" to specify dynamic options within the scope of the appended table.

Brief change log

Implement dynamic options in Blink planner
Implement CSV connector
Add plan test OptionsHintTest
Add a Csv ITCase in CatalogTableITCase

Verifying this change

See ITCases and UT tests in the patch.

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): yes
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: no
The S3 file system connector: no

Documentation

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

… planner * Implement dynamic options in Blink planner * Implement CSV connector * Add plan test OptionsHintTest * Add a Csv ITCase in CatalogTableITCase

flinkbot · 2020-04-13T05:19:59Z

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 bffb4b0 (Mon Apr 13 05:19:59 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

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

Details

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-04-13T05:34:05Z

CI report:

3ea01f3 Travis: SUCCESS Azure: SUCCESS
84098c1 Travis: PENDING 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

KurtYoung · 2020-04-13T07:23:46Z

...flink-table-api-java/src/main/java/org/apache/flink/table/api/config/TableConfigOptions.java

+
+/**
+ * This class holds {@link org.apache.flink.configuration.ConfigOption}s used by
+ * Flink's BLINK table planner.


Not necessary to emphasize this

KurtYoung · 2020-04-13T07:25:02Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogTable.java

+	 *
+	 * @return a new copy of this table with replaced table options
+	 */
+	CatalogTable copy(Map<String, String> options);


use properties to be aligned with other places?

As discussed in the mailing list, options is our new preference to describe the table properties.

KurtYoung · 2020-04-13T07:40:29Z

...flink-table-api-java/src/main/java/org/apache/flink/table/catalog/ConnectorCatalogTable.java


+	@Override
+	public CatalogTable copy(Map<String, String> options) {
+		throw new UnsupportedOperationException("ConnectorCatalogTable cannot copy with new table options");


Should we also support the path of TableEnvironment.connect to specify table option?

All the table translating through CatalogTableImpl can have valid dynamic table options, including the table from TableEnvironment.connect. There is no way for a ConnectorCatalogTable to override table options because the table source is already there.

KurtYoung · 2020-04-13T07:46:53Z

...ble-api-java/src/main/java/org/apache/flink/table/operations/CatalogSinkModifyOperation.java

 	private final Map<String, String> staticPartitions;
 	private final QueryOperation child;
 	private final boolean overwrite;
+	private final List<?> tableHints;


why use List<?>?

The module flink-table-api-java has no Calcite as dependency, use List<?> instead of List to fix the compiler warnings.

I think we need to extract useful information from List and place it here.

The RelHint itself is a POJO which has hint name and hint options as attributes, passing through these RelHints and hand-over them to planner for specific manipulation can simplify the codes.

But i'm also fine if we pre-process these RelHints and extract the contents we care about, such as the dynamic options or some other attributes (index in the future) directly in CatalogSinkModifyOperation.

KurtYoung

Mostly LGTM, only 2 minor comments.

KurtYoung · 2020-04-14T13:08:17Z

...planner-blink/src/test/scala/org/apache/flink/table/planner/catalog/CatalogTableITCase.scala

+
+    val query =
+      """
+        |INSERT INTO T2 /*+ OPTIONS('format.field-delimiter' = ',') */


, is actually the default delimiter even if you didn't configure it. Use another one to make sure this take affect.

Agree, thanks.

KurtYoung · 2020-04-14T13:09:07Z

...anner-blink/src/main/scala/org/apache/flink/table/planner/plan/schema/TableSourceTable.scala

+    } else {
+      // Add the dynamic options as part of the table digest,
+      // this is a temporary solution, we expect to avoid this
+      // before Calcite 1.23.0.


Create an issue to track this?

Thanks, i have fired a issue https://issues.apache.org/jira/browse/FLINK-17147

KurtYoung

LGTM, and I played with this feature in sql cli and it works nicely.

[FLINK-17103][table-planner-blink] Supports dynamic options for Blink…

bffb4b0

… planner * Implement dynamic options in Blink planner * Implement CSV connector * Add plan test OptionsHintTest * Add a Csv ITCase in CatalogTableITCase

rmetzger added review=description? component=TableSQL/API labels Apr 13, 2020

KurtYoung reviewed Apr 13, 2020

View reviewed changes

Fix Kurt's review comment address

3ea01f3

KurtYoung reviewed Apr 14, 2020

View reviewed changes

Fix by Kurt's review comment address

84098c1

KurtYoung approved these changes Apr 15, 2020

View reviewed changes

KurtYoung closed this in 0f83584 Apr 15, 2020

[FLINK-17103][table-planner-blink] Supports dynamic options for Blink… #11711

[FLINK-17103][table-planner-blink] Supports dynamic options for Blink… #11711

Uh oh!

Conversation

danny0405 commented Apr 13, 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

Uh oh!

flinkbot commented Apr 13, 2020

Automated Checks

Review Progress

Uh oh!

flinkbot commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KurtYoung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KurtYoung Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KurtYoung left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented Apr 13, 2020 •

edited

Loading

KurtYoung Apr 14, 2020 •

edited

Loading