Skip to content

[MINOR][SS][DOCS] Adapt multiple watermark policy comment to the reality#25832

Closed
bartosz25 wants to merge 1 commit into
apache:masterfrom
bartosz25:fix_comments_multiple_watermark_policy
Closed

[MINOR][SS][DOCS] Adapt multiple watermark policy comment to the reality#25832
bartosz25 wants to merge 1 commit into
apache:masterfrom
bartosz25:fix_comments_multiple_watermark_policy

Conversation

@bartosz25

@bartosz25 bartosz25 commented Sep 18, 2019

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Previous comment was true for Apache Spark 2.3.0. The 2.4.0 release brought multiple watermark policy and therefore stating that the 'min' is always chosen is misleading.

This PR updates the comments about multiple watermark policy. They aren't true anymore since in case of multiple watermarks, we can configure which one will be applied to the query. This change was brought with Apache Spark 2.4.0 release.

Why are the changes needed?

It introduces some confusion about the real execution of the commented code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

The tests weren't added because the change is only about the documentation level. I affirm that the contribution is my original work and that I license the work to the project under the project's open source license.

previous comment was true for Apache Spark prior 2.4.0. The 2.4.0 release brought multiple watermark policy and therefore stating that the 'min' is always chosen is misleading.
@dongjoon-hyun

Copy link
Copy Markdown
Member

Thank you for your first contribution, @bartosz25 . I updated your PR title a little.

@dongjoon-hyun dongjoon-hyun changed the title Adapt multiple watermark policy comment to the reality [MINOR][SQL][DOCS] Adapt multiple watermark policy comment to the reality Sep 18, 2019
@dongjoon-hyun

Copy link
Copy Markdown
Member

ok to test

@dongjoon-hyun dongjoon-hyun changed the title [MINOR][SQL][DOCS] Adapt multiple watermark policy comment to the reality [MINOR][SS][DOCS] Adapt multiple watermark policy comment to the reality Sep 18, 2019
@dongjoon-hyun dongjoon-hyun removed the SQL label Sep 18, 2019
@bartosz25

Copy link
Copy Markdown
Contributor Author

Thank you @dongjoon-hyun

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master/branch-2.4.
This is a comment-only change and GitHub Action and Jenkins compilation test passed already.

dongjoon-hyun pushed a commit that referenced this pull request Sep 18, 2019
### What changes were proposed in this pull request?

Previous comment was true for Apache Spark 2.3.0. The 2.4.0 release brought multiple watermark policy and therefore stating that the 'min' is always chosen is misleading.

This PR updates the comments about multiple watermark policy. They aren't true anymore since in case of multiple watermarks, we can configure which one will be applied to the query. This change was brought with Apache Spark 2.4.0 release.

### Why are the changes needed?

It introduces some confusion about the real execution of the commented code.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

The tests weren't added because the change is only about the documentation level. I affirm that the contribution is my original work and that I license the work to the project under the project's open source license.

Closes #25832 from bartosz25/fix_comments_multiple_watermark_policy.

Authored-by: bartosz25 <bartkonieczny@yahoo.fr>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit b4b2e95)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@SparkQA

SparkQA commented Sep 18, 2019

Copy link
Copy Markdown

Test build #110925 has finished for PR 25832 at commit cec8296.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants