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-32474][table] Support time travel in table planner #22939

Merged
merged 8 commits into from
Jul 21, 2023
Merged

[FLINK-32474][table] Support time travel in table planner #22939

merged 8 commits into from
Jul 21, 2023

Conversation

hackergin
Copy link
Contributor

What is the purpose of the change

*Support time travel *

Brief change log

  • When validating, parse the Snapshot information to obtain the Schema corresponding to the Snapshot.
  • Convert RelNode will also use Snapshot information to generate the corresponding Schema corresponding to the Snapshot.
    • Modify the relevant implementation of Flink Schema to adapt to the Snapshot usage scenario of Calcite.*

Verifying this change

Unit Test

  • Test time travel syntax with constant Timestamp String
  • Test time travel syntax with simple expression scenarios

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, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? ( JavaDocs)
  • More detailed documentation will be provided separately in Jira.

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 4, 2023 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

luoyuxia
luoyuxia reviewed Jul 4, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@hackergin Thanks for the pr. I left some commets. PTAL.

...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/ScopeSnapshot.java Outdated Show resolved Hide resolved
...table-planner/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespaceSnapshot.java Outdated Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...ner/src/main/java/org/apache/flink/table/planner/plan/FlinkCalciteCatalogReaderSnapshot.java Outdated Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/ScopeSnapshot.java Outdated Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/ScopeSnapshot.java Outdated Show resolved Hide resolved
@luoyuxia
Copy link
Contributor

luoyuxia commented Jul 4, 2023

Btw, the test fails. Please make sure the test pass.

@hackergin
Copy link
Contributor Author

@luoyuxia Thank you very much for your great suggestion which I learn a lot . I update the code, please have a look when you are free.

luoyuxia
luoyuxia reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@hackergin Thanks for updating. I left some comments. PTAL.

...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java Show resolved Hide resolved
...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java Outdated Show resolved Hide resolved
...table-planner/src/main/java/org/apache/calcite/sql/validate/IdentifierSnapshotNamespace.java Outdated Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SnapshotScope.java Outdated Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SnapshotScope.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...planner/src/test/java/org/apache/flink/table/planner/runtime/batch/sql/TimeTravelITCase.java Outdated Show resolved Hide resolved
...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java Outdated Show resolved Hide resolved
@hackergin hackergin force-pushed the FLINK-32474 branch 2 times, most recently from d7150ab to e598fd4 Compare July 18, 2023 06:08
luoyuxia
luoyuxia reviewed Jul 18, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@hackergin Thanks for updating. The pr generally looks good to me. I left minor comment. Should be ready to be merge in next few iterations.

...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SnapshotScope.java Outdated Show resolved Hide resolved
...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SnapshotScope.java Outdated Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
luoyuxia
luoyuxia reviewed Jul 20, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

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

@hackergin Thanks for updating. I left minor comments. PTAL.
Should be ready to be merged in next iteration.

flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
...e-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java Outdated Show resolved Hide resolved
.../src/main/scala/org/apache/flink/table/planner/plan/nodes/logical/FlinkLogicalSnapshot.scala Outdated Show resolved Hide resolved
...le-planner/src/test/java/org/apache/flink/table/planner/factories/TestTimeTravelCatalog.java Outdated Show resolved Hide resolved
...le-planner/src/test/java/org/apache/flink/table/planner/factories/TestTimeTravelCatalog.java Outdated Show resolved Hide resolved
...le-planner/src/test/java/org/apache/flink/table/planner/factories/TestTimeTravelCatalog.java Outdated Show resolved Hide resolved
...le-planner/src/test/java/org/apache/flink/table/planner/factories/TestTimeTravelCatalog.java Outdated Show resolved Hide resolved
luoyuxia
luoyuxia approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment •
edited

Choose a reason for hiding this comment

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

@hackergin Thanks for the update. LGTM! Please ping me after test pass. I'll help merge.

@luoyuxia
Copy link
Contributor

But I think it may well fail since FLINK-32632, anyway, ping me after test complelte

@hackergin
Copy link
Contributor Author

@luoyuxia Thanks for your review. The test completed, and failed since FLINK-32632

@luoyuxia luoyuxia merged commit 410b52a into apache:master Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luoyuxia luoyuxia luoyuxia approved these changes

Assignees
No one assigned
Labels
component=TableSQL/API
Projects
None yet
Milestone
No milestone
3 participants
@hackergin @flinkbot @luoyuxia