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.

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

[FLINK-19530] Reorganise contents of Table API concepts #14257

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

KKcorps
Copy link

@KKcorps KKcorps commented Nov 29, 2020

The PR involves the following changes -

  • Move dynamic tables, time attributes and joins page under a new unbounded data processing section
  • Rewrite and add new content to Dynamic Tables and Joins Page.
  • Move the Query Configuration inside the Dynamic Tables page.

@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 dd3187b (Sun Nov 29 06:01:04 UTC 2020)

Warnings:

  • Invalid pull request title: No valid Jira ID provided

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

@KKcorps KKcorps changed the title streaming concepts [FLINK-19530] Reorganise contents of Table API concepts Nov 29, 2020
@flinkbot
Copy link
Collaborator

flinkbot commented Nov 29, 2020

CI report:

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

Copy link
Contributor

@sjwiesman sjwiesman left a comment

Choose a reason for hiding this comment

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

I am going to do a proper review tomorrow but a few notes:

  1. There is a merge conflict that needs to be resolved, please rebase on master
  2. In general, please put the ticket in the commit message
  3. Please move the files back to where they were. Everything under the streaming folder is relevant for unbounded processing and moving the files breaks links to our docs. I don't think there is any need for the additional subsection.

After [defining temporal table function](temporal_tables.html#defining-temporal-table-function), we can start using it.
Temporal table functions can be used in the same way as normal table functions would be used.
To simplify such queries, Flink offers [Temporal Functions](../temporal_tables.html). For this example, we define a temporal function `Rates` as follows :
//TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't merge in todo's

Copy link
Author

Choose a reason for hiding this comment

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

My bad. Seems like IntelliJ settings got messed up. Removing them manually.

@@ -36,6 +36,14 @@ For the changing dimension table, Flink allows for accessing the content of the
Motivation
----------
Copy link
Contributor

Choose a reason for hiding this comment

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

temporal tables got a lot of new features in 1.12. Please rebase, unfortunately, a lot of what you did is no longer relevant.


For more information regarding the syntax, please check the join sections in [Table API](../tableApi.html#joins) and [SQL]({{ site.baseurl }}/dev/table/sql/queries.html#joins).

//TODO: Explain the complications of unbounded joins
Copy link
Contributor

Choose a reason for hiding this comment

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

remove todo

Comment on lines +35 to 37
Regular Joins and its challenges
---------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets get rid of "and its challenges". We want to make flink more approachable :)

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

@@ -351,4 +339,20 @@ FROM

<span class="label label-danger">Attention</span> Flink does not support event time temporal table joins currently.

### Temporal Table joins vs Other joins

In contrast to [regular joins](#regular-joins), the previous results of the temporal table join will not be affected despite the changes on the build side. Also, the temporal table join operator is very lightweight and does not keep any state.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also doesn't sound right here.

Suggested change
In contrast to [regular joins](#regular-joins), the previous results of the temporal table join will not be affected despite the changes on the build side. Also, the temporal table join operator is very lightweight and does not keep any state.
In contrast to [regular joins](#regular-joins), the previous results of the temporal table join will not be affected despite the changes on the build side. The temporal table join operator is very lightweight and does not keep any state.


In contrast to [regular joins](#regular-joins), the previous results of the temporal table join will not be affected despite the changes on the build side. Also, the temporal table join operator is very lightweight and does not keep any state.

Compared to [interval joins](#interval-joins), temporal table joins do not define a time window within which the records will be joined.
Copy link
Contributor

Choose a reason for hiding this comment

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

again, 1.12 added support for temporal event time table joins so this needs to be updated.

@KKcorps
Copy link
Author

KKcorps commented Dec 3, 2020

I am going to do a proper review tomorrow but a few notes:

  1. There is a merge conflict that needs to be resolved, please rebase on master
  2. In general, please put the ticket in the commit message
  3. Please move the files back to where they were. Everything under the streaming folder is relevant for unbounded processing and moving the files breaks links to our docs. I don't think there is any need for the additional subsection.

The FLIP-60 mentions a new section for unbounded data processing. I Will revert to older structure.

@aljoscha aljoscha removed their request for review March 16, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants