[FLINK-10755][table] Port external catalogs in Table API extension points to flink-table-common #7848

dianfu · 2019-02-27T13:07:37Z

What is the purpose of the change

This pull request ported external catalog to flink-table-common

Brief change log

Ported the following classes to flink-table-common: ExternalCatalog, ExternalCatalogTable, CatalogNotExistException, TableNotExistException
Remove method ExternalCatalogTable.builder()

Verifying this change

This change is a code cleanup without any test coverage.

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, 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? (not applicable)

flinkbot · 2019-02-27T13:08:22Z

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.

Review Progress

✅ 1. The [description] looks good.
- Approved by @sunjincheng121 [committer], @twalthr [PMC]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @sunjincheng121 [committer], @twalthr [PMC]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @twalthr [PMC]
✅ 5. Overall code [quality] is good.
- Approved by @twalthr [PMC]

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

dianfu · 2019-02-27T13:09:55Z

@twalthr Have created a PR to port external catalogs to flink-table-common. Could you help to take a look when it's convenient for you? Thanks in advance. :)

bowenli86

Thanks for working on this PR!

Just share my thought on the annotation. I don't think we should add annotations to existing catalog related classes, given 1) they are not annotated before, 2) they will soon be replaced by new catalog APIs

bowenli86 · 2019-02-27T22:18:06Z

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

+ * <p>It provides information about catalogs, databases and tables such as names, schema,
+ * statistics, and access information.
+ */
+@PublicEvolving


remove annotation

bowenli86 · 2019-02-27T23:49:06Z

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

+ *
+ * <p>Use {@link ExternalCatalogTableBuilder} to integrate with the normalized descriptor-based API.
+ */
+@PublicEvolving


remove annotation

dianfu · 2019-02-28T05:45:11Z

@bowenli86 Thanks a lot for the suggestions. I'm fine to remove the annotation of
ExternalCatalog and ExternalCatalogTable if they will be replaced with new Catalog API as these classes may be refactored or removed at that time. Regarding to the annotation itself, I think it's needed regardless whether it exists before. Porting classes is a good opportunity to correct the annotation which may be not marked correctly before. @twalthr What's your thought?

sunjincheng121 · 2019-03-01T01:24:55Z

@flinkbot approve description
@flinkbot approve consensus

Should be rebased after all commits of FLINK-11449 be merged.

dianfu · 2019-03-01T01:52:56Z

@sunjincheng121 Thanks a lot for the review. I will rebase the PR after
all the commits of FLINK-11449 merged. :)

dianfu · 2019-03-15T01:50:06Z

@sunjincheng121 @twalthr I have rebased the PR since FLINK-11449 has been merged. Could you help to take a look at this PR? Thanks in advance.

twalthr · 2019-03-15T13:33:16Z

@bowenli86 thanks for your suggestion. I agree that the catalog API will change in the near future but this doesn't mean that we can't annotate it. The annotation allows users to know what is intended to be used or not. The current catalog APIs are still intended to be used. The Evolving tells people that it might change again. Additionally, the annotation tells contributors when to think twice about changing a class.

bowenli86 · 2019-03-15T19:37:29Z

@twalthr re: The current catalog APIs are still intended to be used. What my team is thinking of rolling out unified catalog APIs is to build them separately and not touch existing ExternalCatalog APIs, and, upon it having full capabilities of existing catalog APIs, we can just do a switch. That's how we did it internally, and of course we can discuss if you have a different rollout plan. Anyway, the above was the background I had in mind then. With that, I was concerned that japicmp may report errors when changing a class's annotation from PublicEvolving to Deprecated, thus I did some testing locally and it seems to be fine.

I think it's ok to mark them as PublicEvolving. It's a bit hard to say as everything is moving around so fast in Flink table/SQL and our planned timeline.

twalthr · 2019-03-18T09:57:49Z

@bowenli86 Having a completely new interface for the unified catalog APIs and performing the switch later is completely fine. This is also what Xuefu and I had discussed offline. As far as I know, japicmp is not in place for the Table & SQL API.

twalthr

Thank you @dianfu. I added a couple of comments. My biggest concern right now is that this PR does much more than just porting external catalog interfaces but also changes rowtime descriptors and field computer interfaces. If we would skip the ExternalCatalogBuilder as discussed initially we could have avoided porting a lot of classes that might change very soon anyways due to the unified catalog API.

twalthr · 2019-03-18T10:14:56Z

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

+	 * <p>The following example shows how to read from a connector using a JSON format and
+	 * declaring it as a table source:
+	 *
+	 * <p>{@code


Use <pre>{@code …}</pre> instead. See https://reflectoring.io/howto-format-code-snippets-in-javadoc/.

twalthr · 2019-03-18T10:29:33Z

...able/flink-table-planner/src/main/scala/org/apache/flink/table/sources/TableSourceUtil.scala

      // add cast to requested type and convert expression to RexNode
-      // TODO we cast to planner expressions as a temporary solution to keep the old interfaces
-      val rexExpression = Cast(expression.asInstanceOf[PlannerExpression], resultType)
+      val rexExpression = ApiExpressionUtils


I would leave a TODO here as it reminds us that this is still not the final solution.

twalthr · 2019-03-18T10:32:44Z

flink-connectors/flink-connector-elasticsearch6/pom.xml

 		</dependency>

+		<dependency>
+			<groupId>org.apache.flink</groupId>


Can we remove dependencies to flink-table-api-java-bridge or flink-table-planner already now? I think the elasticsearch module should be able to work with flink-table-common classes only, right?

It still depends on flink-table-api-java-bridge and flink-table-planner as Elasticsearch6UpsertTableSinkFactory implements StreamTableSinkFactory.

twalthr · 2019-03-18T10:34:44Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/descriptors/Rowtime.java

+	 * @param extractor The {@link TimestampExtractor} to extract the rowtime attribute
+	 *                  from the physical type.
+	 */
+	public Rowtime timestampsFromExtractor(TimestampExtractor extractor) {


This changes the original logic. Before we were only serializing as the last option. Serializing is dangerous as we might not be able to restore the base64 string. Please reintroduce the original logic.

twalthr · 2019-03-18T10:35:02Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/descriptors/Rowtime.java

+	 * Sets a custom watermark strategy to be used for the rowtime attribute.
+	 */
+	public Rowtime watermarksFromStrategy(WatermarkStrategy strategy) {
+		internalProperties.putString(


This changes the original logic. Before we were only serializing as the last option. Serializing is dangerous as we might not be able to restore the base64 string. Please reintroduce the original logic.

twalthr · 2019-03-18T10:35:30Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/descriptors/Rowtime.java

+	private final DescriptorProperties internalProperties = new DescriptorProperties(true);
+
+	/**
+	 * These constants will be removed once RowtimeValidator is ported to flink-table-common.


Is it too much effort to port the RowtimeValidator now?

twalthr · 2019-03-18T10:36:02Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/descriptors/Schema.java

+	 * Specifies the origin of the previously defined field. The origin field is defined by a
+	 * connector or format.
+	 *
+	 * <p>E.g. field("myString", Types.STRING).from("CSV_MY_STRING")


Use {@code field(...} for all code exmaples in those methods.

twalthr · 2019-03-18T10:42:10Z

flink-table/flink-table-common/src/main/java/org/apache/flink/table/sources/FieldComputer.java

+	 * @param fieldAccesses Field access expressions for the argument fields.
+	 * @return The expression to extract the timestamp from the {@link TableSource} return type.
+	 */
+	public abstract Expression getExpression(FieldReferenceExpression[] fieldAccesses);


I had this discussion with @sunjincheng121 before that this breaks the current interfaces and also changes the serialization format of serialized timestamp extractors. I'm not happy with this changes but I also see that we need progress here.

In any case, I would like to get such breaking changes communicated through the description of the PR.

If we would simply postpone the ExternalCatalogBuilder porting, most of the work of this PR would not have been necessary.

twalthr · 2019-03-18T10:43:42Z

...able/flink-table-planner/src/test/scala/org/apache/flink/table/descriptors/RowtimeTest.scala

        "50YWJsZS5zb3VyY2VzLndtc3RyYXRlZ2llcy5QdW5jdHVhdGVkV2F0ZXJtYXJrQXNzaWduZXKBUc57oaWu9A" +
        "IAAHhyAD1vcmcuYXBhY2hlLmZsaW5rLnRhYmxlLnNvdXJjZXMud21zdHJhdGVnaWVzLldhdGVybWFya1N0cm" +
-        "F0ZWd5mB_uSxDZ8-MCAAB4cA")
+        "F0ZWd57RqMYVyVWhUCAAB4cA")


We should not break tests in "porting" pull requests.

dianfu · 2019-03-18T12:18:51Z

@twalthr Thanks a lot for the review. Considering that this change will break the API of FieldComputer and the serialization of Rowtime, I'd like to take the original solution and avoid the changes of ExternalCatalogBuilder. What do you think? I intended to avoid removing the method ExternalCatalogTable.builder() if possible (as done in the current PR), but it seems that removing it would be a nicer solution.

…ints to flink-table-common

twalthr · 2019-03-18T14:03:25Z

@dianfu thanks for your feedback. Yes, I think we should always aim for small and less invasive steps. The external catalog interfaces will be deprecated anyway soon. If we change the rowtime descriptor and field computer interfaces, we should do it as part of a big table source refactoring. The most important goal is to unblock Table and TableEnvironment refactoring tasks.

dianfu · 2019-03-18T14:10:31Z

@twalthr Makes much sense to me. I have updated the PR and limit the changes to a minimal. Looking forward to your feedback. :)

twalthr

Thanks for the update @dianfu. The changes look good to me. Will merge...
@flinkbot approve all

…ints to flink-table-common This closes apache#7848.

rmetzger added the review=description? label Feb 27, 2019

bowenli86 reviewed Feb 27, 2019

View reviewed changes

dianfu force-pushed the port_ExternalCatalog branch from 3dcfe1f to 7d51f47 Compare February 28, 2019 04:45

sunjincheng121 self-requested a review March 1, 2019 01:25

rmetzger added review=architecture? and removed review=description? labels Mar 1, 2019

dianfu force-pushed the port_ExternalCatalog branch from 7d51f47 to 423f83a Compare March 14, 2019 08:28

dianfu force-pushed the port_ExternalCatalog branch from f966ea9 to 683b836 Compare March 15, 2019 13:32

twalthr requested changes Mar 18, 2019

View reviewed changes

[FLINK-10755][table] Port external catalogs in Table API extension po…

1f5e941

…ints to flink-table-common

dianfu force-pushed the port_ExternalCatalog branch from 683b836 to 1f5e941 Compare March 18, 2019 14:02

remove unnecessary change to flink-connectors

6b7ba9a

rmetzger added the component=API/TableSQL label Mar 18, 2019

twalthr approved these changes Mar 19, 2019

View reviewed changes

rmetzger added review=approved ✅ and removed review=architecture? labels Mar 19, 2019

asfgit closed this in 1eb818b Mar 19, 2019

HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Mar 20, 2019

[FLINK-10755][table] Port external catalogs in Table API extension po…

512bd4f

…ints to flink-table-common This closes apache#7848.

HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Apr 22, 2019

[FLINK-10755][table] Port external catalogs in Table API extension po…

a367bbe

…ints to flink-table-common This closes apache#7848.

sunhaibotb pushed a commit to sunhaibotb/flink that referenced this pull request May 8, 2019

[FLINK-10755][table] Port external catalogs in Table API extension po…

418f969

…ints to flink-table-common This closes apache#7848.

dianfu deleted the port_ExternalCatalog branch June 10, 2020 02:48

flinkbot added component=TableSQL/API and removed component=API/TableSQL labels Mar 17, 2022

[FLINK-10755][table] Port external catalogs in Table API extension points to flink-table-common #7848

[FLINK-10755][table] Port external catalogs in Table API extension points to flink-table-common #7848

Uh oh!

Conversation

dianfu commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Progress

Uh oh!

dianfu commented Feb 27, 2019

Uh oh!

bowenli86 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!

dianfu commented Feb 28, 2019

Uh oh!

sunjincheng121 commented Mar 1, 2019

Uh oh!

dianfu commented Mar 1, 2019

Uh oh!

dianfu commented Mar 15, 2019

Uh oh!

twalthr commented Mar 15, 2019

Uh oh!

bowenli86 commented Mar 15, 2019

Uh oh!

twalthr commented Mar 18, 2019

Uh oh!

twalthr 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!

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!

dianfu commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twalthr commented Mar 18, 2019

Uh oh!

dianfu commented Mar 18, 2019

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dianfu commented Feb 27, 2019 •

edited

Loading

flinkbot commented Feb 27, 2019 •

edited

Loading

dianfu commented Mar 18, 2019 •

edited

Loading