Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FLINK-16264][table] Fix ConnectTableDescriptor loose time attribute bug #11204

Merged
merged 3 commits into from Feb 25, 2020
Merged

[FLINK-16264][table] Fix ConnectTableDescriptor loose time attribute bug #11204

merged 3 commits into from Feb 25, 2020

Conversation

JingsongLi
Copy link
Contributor

What is the purpose of the change

In CatalogTableImpl.fromProperties, we can not remove all schema.* for TableSchema, because Schema (it is a descriptor) is not same to TableSchema. Schema contains time attribute, so we need keep them in properties.

Brief change log

Remove per key for TableSchema in CatalogTableImpl.fromProperties.
Add test ConnectTableDescriptorTest.

Verifying this change

ConnectTableDescriptorTest.

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/Mesos, 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)

@flinkbot
Copy link
Collaborator

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 25f8beb (Tue Feb 25 03:16:49 UTC 2020)

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

@wuchong
Copy link
Member

wuchong commented Feb 25, 2020

Thanks @JingsongLi . I think FLINK-16265 is also introduced by FLINK-15912. We should fix that too.

@flinkbot
Copy link
Collaborator

flinkbot commented Feb 25, 2020 •
edited

CI report:

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

@JingsongLi
Copy link
Contributor Author

Thanks @JingsongLi . I think FLINK-16265 is also introduced by FLINK-15912. We should fix that too.

I created #11205 to fix FLINK-16265.

wuchong
wuchong reviewed Feb 25, 2020
View reviewed changes
...le-api-java/src/test/java/org/apache/flink/table/descriptors/ConnectTableDescriptorTest.java
/**
* Test for {@link ConnectTableDescriptor}.
*/
public class ConnectTableDescriptorTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add a verification in org.apache.flink.table.client.gateway.local.ExecutionContextTest#testTemporalTables to check that the tableEnv.from("EnrichmentSource").getTableSchema contains the rowtime field. It can cover the bug path.

It's not clear what the purpose of this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no rowtime field in tableEnv.from("EnrichmentSource").getSchema. Rowtime field needs special handle in connector codes when translate select ... query.

This test is just for making sure CatalogTable should be same to properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a rowtime field in tableEnv.from("EnrichmentSource").getSchema, because DummyTableSourceFactory in sql-client handles this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not.
It is catalog table, not ConnectorCatalogTable.
Real invoking is in StreamTableSourceScan.translateToPlan, it is hard to validate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it is from SQL-CLI, it is ConnectorCatalogTable now, added test.

wuchong
wuchong reviewed Feb 25, 2020
View reviewed changes
...nt/src/test/java/org/apache/flink/table/client/gateway/utils/TestTableSourceFactoryBase.java
@@ -100,7 +100,7 @@ public TestTableSourceFactoryBase(String type, String testProperty) {
public StreamTableSource<Row> createTableSource(TableSourceFactory.Context context) {
TableSchema schema = context.getTable().getSchema();
final DescriptorProperties params = new DescriptorProperties(true);
params.putProperties(context.getTable().getProperties());
params.putProperties(context.getTable().toProperties());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change this? Is it a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, SchemaValidator.deriveRowtimeAttributes need properties contains TableSchema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class was createTableSource(properties), we should revert to that properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I see.

wuchong
wuchong approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@JingsongLi
Copy link
Contributor Author

Travis passed in: https://travis-ci.org/JingsongLi/flink/builds/654822147

@JingsongLi JingsongLi merged commit dd288c9 into apache:master Feb 25, 2020
@JingsongLi
Copy link
Contributor Author

Thanks @wuchong for reviewing, merged.

@JingsongLi JingsongLi deleted the e2ebug branch April 26, 2020 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wuchong wuchong wuchong approved these changes

Assignees
No one assigned
Labels
component=TableSQL/Client component=Tests review=description?
Projects
None yet
Milestone
No milestone
4 participants
@JingsongLi @flinkbot @wuchong @rmetzger