[FLINK-20551][docs] Make SQL documentation Blink only #14594
Conversation
sjwiesman
requested review from
twalthr and
wuchong
and removed request for
twalthr
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit bdfba9d (Fri May 28 07:11:07 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe 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 commandsThe @flinkbot bot supports the following commands:
|
|
@twalthr could you please take a look at this whenever you have a spare cycle. |
twalthr
left a comment
twalthr
left a comment
There was a problem hiding this comment.
Thanks for the PR @sjwiesman. I added some feedback.
docs/dev/table/common.md
Outdated
| from pyflink.table import EnvironmentSettings, BatchTableEnvironment | ||
|
|
||
| b_b_settings = EnvironmentSettings.new_instance().use_blink_planner().in_batch_mode().build() | ||
| b_b_t_env = BatchTableEnvironment.create(environment_settings=b_b_settings) |
There was a problem hiding this comment.
Isn't this using the legacy planner in the background? I thought the Python API has also a unified environment, no?
There was a problem hiding this comment.
That's a great question, I need to check on that
There was a problem hiding this comment.
Unfortunately, PyFlink does not have a way of instantiating a pure TableEnvironment. I did verify this uses the blink planner and opened a ticket to add this functionality.
| val stream: DataStream[Long] = ... | ||
|
|
||
| // convert DataStream into Table with default field name "f0" | ||
| // Convert DataStream into Table with default field name "f0" |
There was a problem hiding this comment.
side comment: it's funny how taste differs, I prefer lower case for inline comments :D
|
@twalthr I appreciate the review. I've pushed an update that I hope addresses all of your comments. |
twalthr
left a comment
twalthr
left a comment
There was a problem hiding this comment.
One last comment, otherwise good to merge.
| * Registering a user-defined (scalar, table, or aggregation) function | ||
| * Converting a `DataStream` into a `Table` | ||
| * Holding a reference to a `StreamExecutionEnvironment` | ||
| * In case of StreamTableEnvironment holding a reference to a `StreamExecutionEnvironment` |
There was a problem hiding this comment.
nit: I would surround it with back ticks
|
|
||
| </div> | ||
| <div data-lang="Python" markdown="1"> | ||
| N/A |
There was a problem hiding this comment.
not very nice, but I guess we need a separate issue for this
4 participants
What is the purpose of the change
As Blink has been the default planner for some time, and 1.12 now offers bounded data stream support, we should make the documentation "blink only". I think there is more we can do to improve the docs now that we don't have to worry about the legacy planner but that's a bigger update.
Note to reviewers: I did not update the
.zh.mdpages. So many of them have been translated that I was unable to without just replacing whole sections with English. I will open a follow-up ticket.