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

[BEAM-3749] Add test for a trigger with windowed SQL query #4826

Merged
merged 1 commit into from Mar 12, 2018
Merged

[BEAM-3749] Add test for a trigger with windowed SQL query #4826

merged 1 commit into from Mar 12, 2018

Conversation

kennknowles
Copy link
Member

This adds a test to confirm that Window.configure() can be applied to input to a SQL transform that has windowing and the trigger will be used.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand:
    • What the pull request does
    • Why it does it
    • How it does it
    • Why this approach
  • Each commit in the pull request should have a meaningful subject line and body.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@kennknowles kennknowles requested a review from mingmxu March 7, 2018 22:56
@kennknowles
Copy link
Member Author

@akedin @xumingmin

I didn't end up using all the test utils to build the row because the methods on TestStream are not quite right. I can come back to this and add some methods for TestStream but I wanted to get this out to show the behavior that does work.

akedin
akedin approved these changes Mar 7, 2018
View reviewed changes
Copy link
Contributor

@akedin akedin left a comment

Choose a reason for hiding this comment

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

lgtm

...tensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslAggregationTest.java Outdated
+ " GROUP BY TUMBLE(f_timestamp, INTERVAL '1' HOUR)";

RowType outputRowType = RowSqlType.builder().withIntegerField("fn_int_sum").build();
;
Copy link
Contributor

@akedin akedin Mar 7, 2018 •
edited

Choose a reason for hiding this comment

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

; not needed

@kennknowles
Copy link
Member Author

@xumingmin what do you think? Including these tests will make sure the current functionality is never lost.

mingmxu
mingmxu approved these changes Mar 12, 2018
View reviewed changes
Copy link

@mingmxu mingmxu left a comment

Choose a reason for hiding this comment

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

LGTM for the new test.
It confuses me that the test can pass, but I cannot reproduce it in a sample job.

@kennknowles kennknowles merged commit 793bfac into apache:master Mar 12, 2018
@kennknowles kennknowles deleted the sql-trigger branch April 24, 2018 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingmxu mingmxu mingmxu approved these changes

@akedin akedin akedin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kennknowles @mingmxu @akedin